laserninja commented on PR #12241: URL: https://github.com/apache/gravitino/pull/12241#issuecomment-5247542903
Thanks @nevzheng, @lasdf1234, @roryqi for the reviews and the approvals. Taking the assigned items in order. **Doc updates before this merges (P2 #4).** I will push to this PR: - a sequence diagram for `/plan` → `plan-task` → `/tasks`, covering the federated path and the terminal batch, - an explicit CUJ table saying which journeys Phase 1 serves and which are Phase 2, and - a rewrite of §9 Task Breakdown, which is now stale. Per the split agreed in #12194, Phase 1 ships as four PRs: #12411 (deterministic task order), #12412 (the `/tasks` endpoint and its plumbing, unadvertised), federation forwarding (to open), then #12194 (batching, `PlanTaskCodec`, `scan-plan-task-batch-size`, the `/v1/config` advertisement, user docs). §9 currently reads as if all of it is one PR. **Phase-2 ticket (P1 #3).** I will open it with the discussion here migrated into it rather than restated, sectioned as: manifest entry-range plan tasks (§8.5); the caching design starting from #12254 - keys, invalidation and staleness, multi-replica behavior, failure modes, sizing; load amplification - recompute-per-redeem, concurrent redemptions landing on different replicas, cold node-local cache - together with the mitigation layers (cache, ranges, coalescing on a replica); and MoR delete attachment once `/tasks` reads named manifest slices (§8.5.4), which may fall to Phase 3 depending on sizing. #11583 and LB affinity go in as notes, not as work. **Your Phase-2 open question, (a) versioned encoding or (b) implementation milestone: (b).** A plan task is opaque, nothing persists it, and none outlives the scan that issued it, so replacing offset/limit with ranges is not an IRC API change and needs neither a migration nor a version field (§5.5.3, §8.5.5). The one case that is not free is a rolling deploy, where a plan task minted by an old replica is redeemed by a new one. Phase 1 already gives that the right shape: a payload the decoder does not understand becomes a `404`, not a guess, so the client replans rather than receiving a wrong slice. The Phase-2 doc should state that ranges keep that property instead of leaving it implied. **The cache default (#12254 / #12320).** @lasdf1234 asked on the issue for a PR that flips the default, and #12320 is that and nothing more: `scan-plan-cache-impl` defaults to `LocalScanPlanCache`, an empty string still disables it, capacity 200 and expiry 60 unchanged. @nevzheng, that is deliberately the toggle and not the caching story you asked for - it settles none of keys, invalidation, multi-replica or sizing, all of which stay in the Phase-2 ticket. If you would rather it wait behind that design, say so and I will hold it. The argument for landing it now is that batched redemption replans on every miss, so the toggle is worth more with Phase 1 in than without it. **Meeting.** Yes, useful, mainly to settle where the Phase-1/Phase-2 line sits. I am on US Pacific; the overlap with Beijing that is not unkind to anyone is 17:00-18:00 PT / 09:00-10:00 CST the next day. @lasdf1234 @roryqi any day this week works - send an invite and add @nevzheng. -- 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]
