laserninja commented on code in PR #12194:
URL: https://github.com/apache/gravitino/pull/12194#discussion_r3660240939
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -484,6 +492,46 @@ public PlanTableScanResponse planTableScan(
}
}
+ /**
+ * Fetch the scan tasks associated with a {@code plan-task} token previously
handed out by {@link
+ * #planTableScan}.
+ *
+ * <p>Scan planning here is synchronous: {@link #planTableScan} always
returns {@code COMPLETED}
+ * with the full set of {@code file-scan-tasks} inline and never emits
{@code plan-tasks} tokens.
+ * A client therefore has no token to present, and any token reaching this
method did not
+ * originate from this server (or came from a server generation that no
longer retains it). Per
+ * the Iceberg REST specification that case is an unknown plan task, so this
method always throws
+ * {@link NoSuchPlanTaskException}, which the REST layer maps to 404.
+ *
+ * <p>The endpoint still exists and is advertised in {@code /v1/config}
because clients such as
+ * pyiceberg refuse to use server-side scan planning at all unless {@code
POST
+ * .../tables/{table}/tasks} is advertised as supported. Implementing it
keeps the two-step
+ * protocol contract intact and gives batched planning a place to land if
{@link #planTableScan}
+ * later starts emitting {@code plan-tasks}.
+ *
+ * @param tableIdentifier the table the plan task belongs to.
+ * @param request the request carrying the {@code plan-task} token.
+ * @return never returns normally.
+ * @throws org.apache.iceberg.exceptions.NoSuchTableException if the table
doesn't exist.
+ * @throws NoSuchPlanTaskException always, since no {@code plan-task} tokens
are ever issued.
+ */
+ public FetchScanTasksResponse fetchScanTasks(
+ TableIdentifier tableIdentifier, FetchScanTasksRequest request) {
+ // Validate the table exists first, so a bad table reports 404 for the
table rather than
+ // masking it as an unknown plan task. Consistent with planTableScan
behavior.
+ getCatalog().loadTable(tableIdentifier);
+
+ LOG.info(
+ "Rejecting unknown plan task '{}' for table {}: scan planning is
synchronous and does not "
+ + "issue plan-task tokens.",
+ request.planTask(),
+ tableIdentifier);
+ throw new NoSuchPlanTaskException(
Review Comment:
@roryqi @lasdf1234 fair point - it is implemented now, and the PR
description is updated.
`POST .../plan` hands a plan to the client in batches of at most
`scan-plan-task-batch-size` file scan tasks (new config, default 1000): the
first batch inline, each remaining batch as a `plan-task` token that `POST
.../tasks` exchanges for its tasks. Plans that fit in one batch are unchanged
and carry no tokens, so the common case is still a single round trip.
Tokens are self-describing rather than server-side state: each carries the
scan request it was planned from, with the snapshot pinned at planning time,
plus the range of tasks it covers. That way a token stays redeemable after a
restart and on any instance serving the catalog - it is resolved from the scan
plan cache when the plan is still cached, and by replanning the pinned snapshot
otherwise (so enabling `scan-plan-cache-impl` is worthwhile for large tables).
Planned tasks are sorted by (file location, start, length) because Iceberg
plans manifests in parallel, so positions stay stable across replans. Unknown,
foreign and no-longer-resolvable tokens still return 404
`NoSuchPlanTaskException`.
For federated catalogs the tokens are minted by the remote catalog, so
`FederatedCatalogWrapper` forwards `POST .../tasks` upstream untouched.
Two things worth flagging while reviewing:
- Batching is on by default, so `/plan` on a table with more than 1000 file
scan tasks now returns tokens instead of everything inline.
`scan-plan-task-batch-size=0` restores the old behavior.
- Scan responses now list the delete files their tasks reference. Iceberg
serializes `delete-file-references` as indexes into that list, so before this
change a plan of a merge-on-read table could not be serialized at all.
Covered by `TestScanPlanTaskBatching`, `TestPlanTaskToken`,
`TestIcebergFetchScanTasksEndpoint` (end-to-end over HTTP), and a federated
delegation test.
--
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]
