lasdf1234 commented on PR #13386:
URL: https://github.com/apache/gravitino/pull/13386#issuecomment-5775466897
> Thanks Sai. On your question in Slack: keep the four policies separate,
one per activity. They attach at different grains and run on different cadences.
>
> My main comment is on the trigger model. The design uses the commit as the
only trigger, and the review checklist in §9.2 rules out a schedule ("no
CronJob"). I want to change that.
>
> A commit is a good signal that a table might need compaction. It is the
wrong clock for the rest:
>
> * Snapshot expiration, manifest rewrite and orphan cleanup are driven by
time and accumulated state. A table that stops committing is never expired or
cleaned, and orphan files come from failed writes, which produce no commit
event.
> * The event path in §5.4 does not refresh statistics, so each trigger
decision uses whatever the last statistics job wrote.
> * Each (table, policy) pair decides independently at commit time. Nothing
ranks tables against each other or caps concurrent jobs, so the busiest tables
get maintained first rather than the ones in worst shape, and a burst of
commits can submit a burst of Spark jobs.
> * A missed event has no recovery path: a process exit between commit and
callback, or a commit that reaches the backing catalog outside our Iceberg REST
server.
>
> The per-commit cost also lands in the entity store. Every commit does an
event insert, a state upsert and a claim update per policy, to reach a decision
that is "not yet" almost every time. At one commit every five seconds with four
policies, that is roughly 10,000 writes an hour for at most one job.
>
> What I want instead:
>
> 1. A commit marks the table dirty, cheaply, with no per-policy work on the
commit path.
> 2. A scheduler runs on a configurable cadence, takes dirty tables,
refreshes their statistics, evaluates the policies, and submits work
worst-first within a concurrency limit and a maintenance window. Tables not
reached carry over to the next run.
> 3. The same scheduler runs the time-driven activities across every table
with an attached policy, not only tables that committed.
> 4. Within a table the order is compaction, manifest rewrite, snapshot
expiration, orphan cleanup, so expiry removes the snapshots compaction creates
and orphan cleanup collects whatever a failed compaction leaves behind.
>
> The commit signal stays useful as an accelerator for compaction on busy
tables. It should not be the only path.
>
> Two smaller items:
>
> * §6.3: `table_maintenance_event` grows by one row per commit and is only
deleted when the table is dropped. It needs retention, or it can go entirely if
`last_job_id` already answers the min-interval question.
> * §5.4: evaluation must not run on the commit thread. The bounded executor
should be the design, not an implementation detail.
Thank you very much for your review, Mark. I agree with your point of view.
However, the time is too close for the next launch, and there are also ten days
of holidays (Mid-Autumn Festival and National Day) in between. Therefore, the
2.0 version will not have scheduled scheduling at present. The subsequent
versions will definitely implement the scheduled scheduling function. I will
also modify the description in the document to say that the scheduled
scheduling will be implemented in the subsequent versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
