laserninja commented on PR #12241: URL: https://github.com/apache/gravitino/pull/12241#issuecomment-5121305424
Thank you for the thorough answers, and for the pointer to how Iceberg itself frames a plan task. All three are folded into the document, which now records them as decisions taken in review rather than open questions (§8), and the implementation follows in #12194. **1. Batching and the cache.** Agreed - caching is out of scope for this design. Batching always applies, and I filed https://github.com/apache/gravitino/issues/12254 to decide whether the scan plan cache should be enabled by default. I put the numbers, the memory-footprint question and the multi-replica caveat in that issue so the discussion has something concrete to work from. **2. The default of 100.** Your reasoning is better than mine, and I have adopted it: gateways and load balancers commonly cap a body at about 1 MB, and 100 file scan tasks land around 0.3 MB, so 100 leaves comfortable headroom. That rationale is now in the document next to the config table (§5.12, §8.2) instead of the bare "matches Iceberg" note, and revisiting it is a follow-up gated on real workloads rather than an open question. **3. Expiry and naming.** You are right that the specification treats it as an opaque string, not a token - "an opaque string provided by the REST server that represents a unit of work for generating file scan tasks for scan planning" - and that framing settles the expiry question: an ordinary request parameter has no lifetime of its own. No expiry, and a plan task now stays redeemable exactly as long as the snapshot it pins exists. I also took the naming point further than the class name, since the token framing had leaked into comments, log messages, the user documentation and the test names. In #12194 (b3560127b): `PlanTaskToken` is now `PlanTaskCodec`, with `PlanTaskCodec.PlanTask` as the decoded unit of work, and "token" is gone from the Iceberg REST server, `docs/iceberg-rest-service.md` and this design document. One thing I would like your read on: `PlanTaskCodec` is a static encode/decode pair, and the thing `decode` returns is the decoded unit of work, which I made a nested `PlanTaskCodec.PlanTask`. The alternative is two top-level classes, `PlanTaskCodec` plus a `PlanTask` value type. I went with the nested class to keep it to one file, but if you prefer the split, it is a small change. -- 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]
