amogh-jahagirdar commented on code in PR #16024:
URL: https://github.com/apache/iceberg/pull/16024#discussion_r3177313116
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java:
##########
@@ -314,6 +322,17 @@ private CloseableIterable<FileScanTask>
fetchPlanningResult() {
return scanTasksIterable(response.planTasks(), response.fileScanTasks());
}
+ private static String failureMessage(String planId, ErrorResponse error) {
+ Preconditions.checkArgument(error != null, "Error must be present for
failed status");
Review Comment:
Minor: I feel like this is a case where we could be lenient on when
consuming the response. Yes it's required by the spec and as @RussellSpitzer
said definitely when we produce responses we should make sure it conforms, but
if a server doesn't send it back we could easily just insert some "unknown"
error context for the message if it's null. It's the same level of complexity
has having a line for the preconditions check too. Then we have a clearer error
message for a failed remote scan planning rather than blowing up due to lack of
error context.
Not a blocker imo, just something to maybe consider.
--
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]
