lasdf1234 commented on PR #13386: URL: https://github.com/apache/gravitino/pull/13386#issuecomment-5807267611
> Sai, the trigger model is right now. Commit drives compaction only, the scheduler is the clock for all four types, orphan is on its own track, and dropping the per-commit event table removes the write load I was worried about. The items below are what still needs to change. > > **1. `next_due_at` is never advanced after a run.** §5.2.5 writes it on policy change and on IRC create/update. §5.4.2 releases to IDLE without changing it. A row that has just run is therefore still due, gets re-claimed on the next 60-second poll, and only `minIntervalMs` stops it, which defaults to one hour. The crontab schedules in §5.2.4 have no effect as written. A successful run must set `next_due_at = nextOccurrence(schedule)`. > > **2. No per-table exclusion.** The claim is per `(table, policy)`, so a compaction rewriting data files and an expiry deleting snapshots can run on the same table at the same time. The requirement is that two maintenance activities never run concurrently on one table. Claim the table for the pass rather than the policy row, or keep per-policy rows and have a claim on any activity block the others for that table. Activities on different tables running concurrently is what we want and stays unaffected. > > **3. Worst-first ordering is missing, and the current shape prevents it.** §5.3.1 selects due candidates with no ranking. Ranking needs a fresh score server side before submission, but §5.4.1 moves update-stats and the decision inside the Spark job. Refresh statistics and evaluate in the claimed window, record the measurements against the table, and let the job execute the decision rather than make it. > > **4. No concurrency cap or maintenance window.** `workerThreads` is per node, so cluster concurrency is 8 times the replica count. Both need to be cluster-wide, with tables not reached carrying to the next run. > > **5. Orphan track is missing both conditions.** §5.6 is one line. It needs the per-table eligibility floor, a table becoming eligible a set interval after its own last successful cleanup with a default of 7 days, so listings spread across the week rather than the whole estate listing on one night. It also needs the `olderThan` floor enforced server side, since policies can be written over REST. Orphan's `minIntervalMs` default of one hour in §7.3 contradicts both. > > **6. The fault-tolerance chapter needs to come back.** Rory asked for the models and I set the choice: at-least-once latest-state evaluation from the scheduler, commit path best effort by design, at most one in-flight job per table per activity, and an idempotency key recorded before submission. Recovery keys on the table's own state, current snapshot id against the id at last measurement, which needs a column on the state row. None of that is in the current revision. > > **7. `minIntervalMs` belongs in the policy, not in `gravitino.conf`.** §7.3 makes it eight server keys. It should live in policy content so it varies per attachment, is visible in the UI, and follows nearest-wins like everything else. > > One scope note: cron expressions are more than 2.0 needs. A fixed interval per policy, with the exactly-once-per-due guarantee across replicas, is enough for the October 16 freeze. Calendars can come later. OK, Mark. I will fix these problems as soon as possible. If we go with the current plan, the cron expression doesn't impose much workload on me. If necessary, could this feature be added as well? -- 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]
