stevenzwu commented on code in PR #16024:
URL: https://github.com/apache/iceberg/pull/16024#discussion_r3170708891
##########
core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java:
##########
@@ -50,6 +54,22 @@ public PlanStatus planStatus() {
return planStatus;
}
+ ErrorResponse errorResponse() {
+ return errorResponse;
+ }
+
+ public String errorMessage() {
+ if (errorResponse == null) {
+ return null;
+ }
+ return String.format(
+ Locale.ROOT,
+ "%s (code=%d): %s",
+ errorResponse.type(),
+ errorResponse.code(),
+ errorResponse.message());
+ }
Review Comment:
Same visibility comment as on `PlanTableScanResponse` — if we flip there,
this should flip here too.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java:
##########
@@ -284,15 +282,18 @@ private CloseableIterable<FileScanTask>
fetchPlanningResult() {
ErrorHandlers.planErrorHandler(),
parserContext);
- if (response.planStatus() == PlanStatus.SUBMITTED) {
- throw new NotCompleteException();
- } else if (response.planStatus() != PlanStatus.COMPLETED) {
- throw new IllegalStateException(
- String.format(
- "Invalid planStatus: %s for planId: %s",
response.planStatus(), id));
+ switch (response.planStatus()) {
+ case COMPLETED:
+ result.set(response);
+ break;
+ case SUBMITTED:
+ throw new NotCompleteException();
+ case FAILED:
+ case CANCELLED:
+ default:
+ throw new IllegalStateException(
+ failureMessage(response.planStatus(), id,
response.errorMessage()));
}
Review Comment:
No test covers the headline behavior of this PR. The description promises:
> the server's error message is surfaced: `Received status: failed for
planId: ... Server error: I am on fire`
…but `TestRESTScanPlanning` doesn't exercise the failed path, so nothing
verifies the exact `IllegalStateException` message this switch throws. The
parser round-trip tests cover the data model but not the user-visible exception
contract.
A mocked-client test for both `planTableScan` (line 220-ish) and this switch
— asserting the thrown message contains the server error — would protect the
contract you're establishing.
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java:
##########
@@ -58,6 +62,22 @@ public String planId() {
return planId;
}
+ ErrorResponse errorResponse() {
+ return errorResponse;
+ }
+
+ public String errorMessage() {
+ if (errorResponse == null) {
+ return null;
+ }
+ return String.format(
+ Locale.ROOT,
+ "%s (code=%d): %s",
+ errorResponse.type(),
+ errorResponse.code(),
+ errorResponse.message());
+ }
Review Comment:
The raw accessor is package-private while the pre-formatted string is public
— that's inverted from how other response classes expose data. Consequences:
- Callers outside this package can't read `code` / `type` / `stack` to
branch on them (e.g., retry on 503 but not 400).
- The format `"%s (code=%d): %s"` becomes part of the public API surface —
anything logging or matching it is locked in.
- `ErrorResponse.toString()` already produces `ErrorResponse(code=500,
type=X, message=...)`; this is a second format callers now have to reconcile.
Suggest: make `errorResponse()` public, drop `errorMessage()`, and let
`RESTTableScan.failureMessage` format directly from the fields it needs. Same
point applies to `FetchPlanningResultResponse`.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java:
##########
@@ -314,6 +315,14 @@ private CloseableIterable<FileScanTask>
fetchPlanningResult() {
return scanTasksIterable(response.planTasks(), response.fileScanTasks());
}
+ private static String failureMessage(PlanStatus status, String planId,
String errorMessage) {
+ if (errorMessage == null) {
+ return String.format("Received status: %s for planId: %s", status,
planId);
+ }
+ return String.format(
+ "Received status: %s for planId: %s. Server error: %s", status,
planId, errorMessage);
+ }
Review Comment:
Two minor things:
1. Once the type and code are embedded inside `errorMessage()`, the `Server
error:` prefix reads awkwardly because the formatted suffix already leads with
a type name (e.g. `... Server error: IllegalStateException (code=500): ...`).
Something like `"Remote scan planning %s for planId: %s: %s"` would read
tighter.
2. If `errorResponse()` were public (see comment on
`PlanTableScanResponse`), this method could format directly from the raw fields
and the whole two-level format decision would be local to its one caller.
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java:
##########
@@ -58,6 +62,22 @@ public String planId() {
return planId;
}
+ ErrorResponse errorResponse() {
+ return errorResponse;
+ }
+
+ public String errorMessage() {
+ if (errorResponse == null) {
+ return null;
+ }
+ return String.format(
+ Locale.ROOT,
+ "%s (code=%d): %s",
+ errorResponse.type(),
+ errorResponse.code(),
+ errorResponse.message());
+ }
Review Comment:
similar to @nastra 's comment above
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java:
##########
@@ -218,11 +218,9 @@ private CloseableIterable<FileScanTask>
planTableScan(PlanTableScanRequest planT
Endpoint.check(supportedEndpoints, Endpoint.V1_FETCH_TABLE_SCAN_PLAN);
return fetchPlanningResult();
case FAILED:
- throw new IllegalStateException(
- String.format("Received status: %s for planId: %s",
PlanStatus.FAILED, planId));
case CANCELLED:
throw new IllegalStateException(
- String.format("Received status: %s for planId: %s",
PlanStatus.CANCELLED, planId));
+ failureMessage(planStatus, planId, response.errorMessage()));
Review Comment:
`case CANCELLED:` is unreachable here — `PlanTableScanResponse.validate()`
already rejects `CANCELLED` (`"'cancelled' is not a valid status for
planTableScan"`). Pre-existing, but since you're restructuring the switch it's
a good moment to drop the branch from this method and keep it only in
`fetchPlanningResult`.
Separately: this contradicts the OpenAPI spec, which maps `cancelled →
EmptyPlanningResult` for `PlanTableScanResult`. Out of scope here, but worth
noting in the PR body.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
