You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rahulsmahadev (via GitHub)" <gi...@apache.org> on 2026/04/02 01:10:17 UTC
[PR] Core: Make REST scan planning poll timeout configurable [iceberg]
rahulsmahadev opened a new pull request, #15863:
URL: https://github.com/apache/iceberg/pull/15863
▎ Summary
▎ - Add configurable rest-scan-planning.poll-timeout-ms catalog property to control the maximum wait time when polling for async scan planning results in RESTTableScan
▎ - Previously hardcoded to 5 minutes, which may not suit all environments (e.g., long-running plans or fast-fail scenarios)
▎ - Default remains 5 minutes for backward compatibility
▎ Test plan
▎ - Added asyncPlanningRespectsConfigurablePollTimeout test in TestRESTScanPlanning that sets a 1ms timeout against a server that never completes planning, verifying the poll loop respects the configured value
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3042573678
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
I don't think this works. We're now assuming a failure is a timeout. How do we distinguish other non-timeout errors?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030596195
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
Agree that we need a semantically clear exception like `RemovePlanTimeoutException`. I'm not sure we're really stuck with this, we can just update it to wrap the Tasks invocation in a try/catch, remap the Not complete to the new failure. We already do that in a few other places like [BaseTransaction](https://github.com/apache/iceberg/blob/1a2a8a5ecc701629183405ca842f2d1eb23505ee/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L378) and [CatalogHandlers](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L815)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030596195
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
Agree that we need a semantically clear exception like `RemovePlanTimeoutException`. I'm not sure we're really stuck with this, we can just update it to wrap the Tasks invocation in a try/catch, remap the Not complete to the new exception type. We already follow that pattern in a few other places like [BaseTransaction](https://github.com/apache/iceberg/blob/1a2a8a5ecc701629183405ca842f2d1eb23505ee/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L378) and [CatalogHandlers](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L815)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031184654
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
I would just remove these comments out about how a client could use it, and just stick with something simple like "Thrown when server-side scan planning times out."
But to answer the question, no I don't think it's a violation. The example I provided was about the case where server side planning is supported and configured for usage by a client but not indicated as required by a server.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030304810
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
This is actually an issue because we're throwing a private exception with no message, which doesn't allow proper handling. This should be something more like `RemotePlanTimeoutException` with a specific error. `NotCompleteException` is not only semantically incorrect, but it isn't distinguishable at all from the calling client as it's only catchable as a RuntimeException.
I guess `Tasks` doesn't really allow us to change the thrown exception, so we're probably stuck with this approach.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031215791
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
IMO I also think this should be in core/rest rather than API. The public API is a more generic implementation agnostic interface (many of those exceptions are either defined in the REST protocol). However, this exception is a bit more specific for our particular client implementation (it's not something generic on the TableScan path). Maybe worth getting different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3029690054
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -49,6 +49,11 @@ private RESTCatalogProperties() {}
public static final String REST_SCAN_PLAN_ID = "rest-scan-plan-id";
+ public static final String REST_SCAN_PLANNING_POLL_TIMEOUT_MS =
+ "rest-scan-planning.poll-timeout-ms";
Review Comment:
Name makes sense to me!
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
I think the reason this asserts isInstanceOf(RuntimeException) is because the `NotCompleteException` is private , yet we bubble it up in case we hit the timeout case.
It's something we can look at separately, but maybe there's a proper public facing execption we want to throw in case of timeouts; not a big deal but that lets clients who have the ability to fall back to client side planning be able to do that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031061879
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
isn't this then violation of server mode being sent as part of loadTable ? would it be better to say try with larger timeout ?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1154,119 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles)
+ .isInstanceOf(RemotePlanTimeoutException.class)
+ .hasMessageContaining("did not complete within configured limits");
+ }
+
+ @Test
+ public void asyncPlanningSucceedsWithCustomTimeout() {
+ configurePlanningBehavior(TestPlanningBehavior.Builder::asynchronous);
+ RESTTable table = restTableFor(restCatalog, "custom_timeout_success");
+ setParserContext(table);
+ // default catalog uses the default 5-minute timeout, which is sufficient for async planning
Review Comment:
The test name is `asyncPlanningSucceedsWithCustomTimeout` but we are relying on default timeout, is this intented ?
[optional] also i would say set the value explicitly in case some one fiddles with the default
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031184654
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
I would just remove these comments out about how a client could use it, and just stick with something simple like "Thrown when server-side scan planning does not complete before the client deadline"
But to answer the question, no I don't think it's a violation. The example I provided was about the case where server side planning is supported and configured for usage by a client but not indicated as required by a server.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031215791
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
IMO I also think this should be in core/rest rather than API. The public API exceptions are more for implementation agnostic exceptions (many of those exceptions are either defined in the REST protocol or just part of the general commit path regardless of catalog implementation).
However, this exception is a bit more specific for our particular REST client implementation for remote scanning (it's not something generic on the TableScan path). Maybe worth getting different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030304810
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
This is actually an issue because we're throwing a private exception with no message, which doesn't allow proper handling. This should be something more like `RemotePlanTimeoutException` with a specific error. `NotCompleteException` is not only semantically incorrect, but it isn't distinguishable at all from the calling client as it's only catchable as a RuntimeException.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3029682196
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
I think the reason this asserts isInstanceOf(RuntimeException) is because the `NotCompleteException` is private , yet we bubble it up in case we hit the timeout case.
It's something we can look at separately, but maybe there's a proper public facing execption we want to throw in case of timeouts; not a big deal but that lets clients who have the ability to fall back to client side planning in such cases be able to do that a bit more cleanly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031220311
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
Thanks for adding @rahulsmahadev! [Left a comment below](https://github.com/apache/iceberg/pull/15863#discussion_r3031215791) on why I think core/rest is a better place for this particular exception rather than the API module. You may want to get different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#issuecomment-4200321592
Merging, thank you @rahulsmahadev , and @singhpk234 @danielcweeks for reviewing!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "rahulsmahadev (via GitHub)" <gi...@apache.org>.
rahulsmahadev commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3034451838
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
updated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #15863:
URL: https://github.com/apache/iceberg/pull/15863
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031215791
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
IMO I also think this should be in core/rest rather than API. The public API exceptions are more for implementation agnostic exceptions along the read/commit path or REST protocol defined exceptions.
However, this exception is a bit more specific for our particular REST client implementation for remote scanning (it's not something generic on the TableScan path). Maybe worth getting different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031215791
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
IMO I also think this should be in core/rest rather than API. The public API exceptions are more for implementation agnostic exceptions along the commit path or REST failures.
However, this exception is a bit more specific for our particular REST client implementation for remote scanning (it's not something generic on the TableScan path). Maybe worth getting different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "rahulsmahadev (via GitHub)" <gi...@apache.org>.
rahulsmahadev commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3034451254
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
thanks added it in core, I think it makes sense there
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core : Make REST scan planning poll timeout configurable [iceberg]
Posted by "rahulsmahadev (via GitHub)" <gi...@apache.org>.
rahulsmahadev commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3034458151
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1154,119 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles)
+ .isInstanceOf(RemotePlanTimeoutException.class)
+ .hasMessageContaining("did not complete within configured limits");
+ }
+
+ @Test
+ public void asyncPlanningSucceedsWithCustomTimeout() {
+ configurePlanningBehavior(TestPlanningBehavior.Builder::asynchronous);
+ RESTTable table = restTableFor(restCatalog, "custom_timeout_success");
+ setParserContext(table);
+ // default catalog uses the default 5-minute timeout, which is sufficient for async planning
Review Comment:
Good catch! Updated the test to create its own catalog with an explicit poll-timeout-ms=30000 instead of relying on the default
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030596195
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
Agree that we need a semantically clear exception like `RemotePlanTimeoutException`. I'm not sure we're really stuck with this, we can just update it to wrap the Tasks invocation in a try/catch, remap the Not complete to the new exception type. We already follow that pattern in a few other places like [BaseTransaction](https://github.com/apache/iceberg/blob/1a2a8a5ecc701629183405ca842f2d1eb23505ee/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L378) and [CatalogHandlers](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L815)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030596195
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
Agree that we need a semantically clear exception like `RemovePlanTimeoutException`. I'm not sure we're really stuck with this, we can just update it to wrap the Tasks invocation in a try/catch, remap the Not complete to the new exception type. We already do that in a few other places like [BaseTransaction](https://github.com/apache/iceberg/blob/1a2a8a5ecc701629183405ca842f2d1eb23505ee/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L378) and [CatalogHandlers](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L815)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "rahulsmahadev (via GitHub)" <gi...@apache.org>.
rahulsmahadev commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3030800134
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTScanPlanning.java:
##########
@@ -1153,6 +1153,76 @@ public void serverSupportsPlanningButNotCancellation() throws IOException {
assertThat(cancelled).isFalse();
}
+ @Test
+ public void asyncPlanningRespectsConfigurablePollTimeout() {
+ // 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);
+
+ 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) {
+ 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-timeout",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ RESTCatalogProperties.SCAN_PLANNING_MODE,
+ RESTCatalogProperties.ScanPlanningMode.SERVER.modeName(),
+ RESTCatalogProperties.REST_SCAN_PLANNING_POLL_TIMEOUT_MS,
+ "1"));
+
+ RESTTable table = restTableFor(catalog, "poll_timeout_test");
+ setParserContext(table);
+ RESTTableScan scan = restTableScanFor(table);
+
+ // With a 1ms timeout and a server that never completes, planFiles should fail
+ assertThatThrownBy(scan::planFiles).isInstanceOf(RuntimeException.class).hasMessage(null);
Review Comment:
I added a new exception to api (not sure if thats the best place) and wrap the exception here, let me know if this makes sense
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Make REST scan planning poll timeout configurable [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #15863:
URL: https://github.com/apache/iceberg/pull/15863#discussion_r3031215791
##########
api/src/main/java/org/apache/iceberg/exceptions/RemotePlanTimeoutException.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception thrown when remote scan planning does not complete within configured limits.
+ *
+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.
Review Comment:
IMO I also think this should be in core/rest rather than API. The public API exceptions are more for implementation agnostic exceptions along the commit path or REST protocol defined exceptions.
However, this exception is a bit more specific for our particular REST client implementation for remote scanning (it's not something generic on the TableScan path). Maybe worth getting different opinions here but just my take.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org