Copilot commented on code in PR #17846:
URL: https://github.com/apache/iceberg/pull/17846#discussion_r3872036692
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1284,6 +1285,140 @@ public void asyncPlanningRejectsInvalidTimeout() {
.hasMessageContaining("must be positive");
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollRetries() {
+ // Create an adapter that always returns SUBMITTED (never completes)
+ List<Endpoint> endpoints =
+ endpointsWithPlanning(
+ Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN,
+ Endpoint.V1_CANCEL_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN_TASKS);
+
+ AtomicInteger fetchAttempts = new AtomicInteger();
+ RESTCatalogAdapter adapter =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ public <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders,
+ ParserContext parserContext) {
+ if (ResourcePaths.config().equals(request.path())) {
+ return castResponse(
+ responseType,
ConfigResponse.builder().withEndpoints(endpoints).build());
+ }
+ T response =
+ super.execute(
+ request, responseType, errorHandler, responseHeaders,
parserContext);
+ if (response instanceof LoadTableResponse) {
+ return castResponse(
+ responseType,
+ withPlanningMode(
+ (LoadTableResponse) response,
+
RESTCatalogProperties.ScanPlanningMode.SERVER.modeName()));
+ }
+
+ // Override fetch responses to always return SUBMITTED so the
poll never completes
+ if (response instanceof FetchPlanningResultResponse) {
+ fetchAttempts.incrementAndGet();
+ return castResponse(
+ responseType,
+ FetchPlanningResultResponse.builder()
+ .withPlanStatus(PlanStatus.SUBMITTED)
+ .build());
+ }
+
+ return response;
+ }
+ });
+
+
adapter.setPlanningBehavior(TestPlanningBehavior.builder().asynchronous().build());
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config)
-> adapter);
+ catalog.initialize(
+ "test-poll-retries",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_NUM_RETRIES,
+ "0"));
+
+ RESTTable table = restTableFor(catalog, "poll_retries_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With 0 retries and a server that never completes, planFiles should fail
after one attempt
+ assertThatThrownBy(scan::planFiles)
+ .isInstanceOf(RemotePlanTimeoutException.class)
+ .hasMessageContaining("did not complete within configured limits");
+ assertThat(fetchAttempts).hasValue(1);
+ }
+
+ @Test
+ public void asyncPlanningSucceedsWithCustomRetries() {
+ List<Endpoint> endpoints =
+ endpointsWithPlanning(
+ Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN,
+ Endpoint.V1_CANCEL_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN_TASKS);
+
+ CatalogWithAdapter catalogWithAdapter =
+ catalogWithEndpoints(endpoints,
TestPlanningBehavior.builder().asynchronous().build());
+
+ catalogWithAdapter.catalog.initialize(
+ "test-custom-retries",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_NUM_RETRIES,
+ "10"));
+
+ RESTTable table = restTableFor(catalogWithAdapter.catalog,
"custom_retries_success");
+ setParserContext(table);
+ assertThat(table.newScan().planFiles()).hasSize(1);
Review Comment:
"10" is the default value for REST_SCAN_PLANNING_POLL_NUM_RETRIES, so this
test can pass even if the property is accidentally ignored. To make the test
actually validate configurability, set a non-default value and assert behavior
that would differ from the default (e.g., complete within a small retry budget,
or instrument fetch calls and assert the expected number of attempts).
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1284,6 +1285,140 @@ public void asyncPlanningRejectsInvalidTimeout() {
.hasMessageContaining("must be positive");
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollRetries() {
+ // Create an adapter that always returns SUBMITTED (never completes)
+ List<Endpoint> endpoints =
+ endpointsWithPlanning(
+ Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN,
+ Endpoint.V1_CANCEL_TABLE_SCAN_PLAN,
+ Endpoint.V1_FETCH_TABLE_SCAN_PLAN_TASKS);
+
+ AtomicInteger fetchAttempts = new AtomicInteger();
+ RESTCatalogAdapter adapter =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ public <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders,
+ ParserContext parserContext) {
+ if (ResourcePaths.config().equals(request.path())) {
+ return castResponse(
+ responseType,
ConfigResponse.builder().withEndpoints(endpoints).build());
+ }
+ T response =
+ super.execute(
+ request, responseType, errorHandler, responseHeaders,
parserContext);
+ if (response instanceof LoadTableResponse) {
+ return castResponse(
+ responseType,
+ withPlanningMode(
+ (LoadTableResponse) response,
+
RESTCatalogProperties.ScanPlanningMode.SERVER.modeName()));
+ }
+
+ // Override fetch responses to always return SUBMITTED so the
poll never completes
+ if (response instanceof FetchPlanningResultResponse) {
+ fetchAttempts.incrementAndGet();
+ return castResponse(
+ responseType,
+ FetchPlanningResultResponse.builder()
+ .withPlanStatus(PlanStatus.SUBMITTED)
+ .build());
+ }
+
+ return response;
+ }
+ });
+
+
adapter.setPlanningBehavior(TestPlanningBehavior.builder().asynchronous().build());
+
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config)
-> adapter);
+ catalog.initialize(
+ "test-poll-retries",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_NUM_RETRIES,
+ "0"));
+
+ RESTTable table = restTableFor(catalog, "poll_retries_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With 0 retries and a server that never completes, planFiles should fail
after one attempt
+ assertThatThrownBy(scan::planFiles)
+ .isInstanceOf(RemotePlanTimeoutException.class)
+ .hasMessageContaining("did not complete within configured limits");
Review Comment:
The PR description calls out that the configured retry limit is included in
the RemotePlanTimeoutException message, but this assertion only checks a
generic substring. Consider also asserting the message includes the configured
value (e.g., "maxRetries=0") so regressions in the diagnostic content are
caught.
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -58,6 +58,10 @@ private RESTCatalogProperties() {}
public static final long REST_SCAN_PLANNING_POLL_TIMEOUT_MS_DEFAULT =
TimeUnit.MINUTES.toMillis(5);
Review Comment:
Consider adding a short comment/Javadoc describing the exact semantics of
"poll-num-retries" (e.g., whether it counts retries in addition to the initial
fetch attempt, and that 0 means "only one fetch attempt"), plus the valid range
(non-negative). This reduces ambiguity for operators tuning the setting.
--
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]
