laserninja commented on PR #12194: URL: https://github.com/apache/gravitino/pull/12194#issuecomment-5246588504
@nevzheng agreed, this PR is too big to review as one change. Here is how I propose to split it, all five tied to #11284. Splitting it also surfaced something I had not noticed: this PR bundles credential vending for `POST .../plan`, which has nothing to do with the `/tasks` endpoint and belongs on its own. | # | PR | What it contains | Size | | - | -- | ---------------- | ---- | | 1 | #12411 | Order planned file scan tasks deterministically | ~190 lines, 2 files | | 2 | to open | Credential vending for `POST .../plan` (`X-Iceberg-Access-Delegation`), and `400` instead of `500` for an empty request body | ~200 lines | | 3 | to open | The `POST .../tasks` endpoint and its plumbing: dispatcher chain, events, audit operation type, `NoSuchPlanTaskException` → `404`. **Not** advertised in `/v1/config` yet | ~350 lines | | 4 | to open | Federated catalogs forward `POST .../tasks` to the remote catalog | ~200 lines | | 5 | this PR | Batching a plan into `plan-tasks`, `PlanTaskCodec`, `scan-plan-task-batch-size`, advertising the endpoint in `/v1/config`, user docs | ~700 lines | Two things about the ordering that are deliberate rather than arbitrary. **The `/v1/config` advertisement lands last, in PR 5.** Clients gate on the advertised endpoint set rather than probing: pyiceberg enables server-side scan planning when it sees `POST .../tasks` in `/v1/config` and fails the scan if the endpoint then does not serve tasks. So advertising it before it can return anything would be worse than not having it at all, and PR 3 deliberately ships the endpoint unadvertised. That also answers @roryqi's point on this PR - PR 3 makes no claim to support the interface; PR 5 is where the claim and the working implementation arrive together. **Federation comes before batching.** A federated catalog delegates `/plan` upstream, so the plan tasks a client receives are the remote catalog's, and forwarding them is correct and testable before Gravitino issues any of its own. Doing it after PR 5 would leave a window where a federated deployment advertises the endpoint and 404s on it. PR 1 is open now. I will open each of the others as its predecessor merges, so that every diff you see is against a base that already has what it depends on, rather than four PRs showing cumulative diffs. This PR stays open as the last one and gets rebased down to slice 5 once 1 to 4 land - the review history here is worth keeping attached to the part it argued about, including the naming discussion with @lasdf1234, the ordering bug @roryqi found, and the manifest entry-range design in #12241. Each PR will be green on `./gradlew :iceberg:iceberg-rest-server:check -PskipITs` on its own, not only at the end of the stack. If you would rather see a different division - for example credential vending folded back in, or the endpoint and batching kept together so the interface is never dormant in `main` - say so and I will reshape it before opening more. -- 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]
