Core : Make REST scan planning poll timeout configurable by rahulsmahadev · Pull Request #15863 · apache/iceberg

rahulsmahadev · 2026-04-02T01:10:14Z

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 (default remains 5 minutes)
Introduce RemotePlanTimeoutException in core/rest (package-private) so the timeout failure is semantically clear and distinguishable from other runtime errors
Wrap the polling loop to catch the internal NotCompleteException and rethrow as RemotePlanTimeoutException with a descriptive message including planId, timeout, and retry limits
Validate that the configured timeout is a positive value

Test plan

asyncPlanningRespectsConfigurablePollTimeout: sets 1ms timeout against a server that never completes, verifies RemotePlanTimeoutException is thrown
asyncPlanningSucceedsWithCustomTimeout: sets an explicit 30s timeout and verifies async planning completes successfully
asyncPlanningRejectsInvalidTimeout: verifies negative timeout values are rejected with IllegalArgumentException

Co-authored-by: Isaac

amogh-jahagirdar

@rahulsmahadev this looks good to me, thank you! I'll leave it up in case @singhpk234 @danielcweeks @nastra or any others have any other comments.

amogh-jahagirdar · 2026-04-02T18:36:06Z

+    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);


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 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.

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 and CatalogHandlers

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

Thanks for adding @rahulsmahadev! Left a comment below 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.

thanks added it in core, I think it makes sense there

I don't think this works. We're now assuming a failure is a timeout. How do we distinguish other non-timeout errors?

amogh-jahagirdar · 2026-04-02T18:37:46Z

  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";


Name makes sense to me!

singhpk234 · 2026-04-03T01:38:04Z

+ * <p>Clients may catch this to fall back to client-side planning when server-side planning is too
+ * slow.


isn't this then violation of server mode being sent as part of loadTable ? would it be better to say try with larger timeout ?

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.

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.

singhpk234 · 2026-04-03T01:57:07Z

+    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


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

Good catch! Updated the test to create its own catalog with an explicit poll-timeout-ms=30000 instead of relying on the default

amogh-jahagirdar

Thanks @rahulsmahadev ! The latest updates look right to me.

amogh-jahagirdar · 2026-04-07T15:46:05Z

Merging, thank you @rahulsmahadev , and @singhpk234 @danielcweeks for reviewing!

Make MAX_WAIT_TIME_MS configurable for RESTTableScan

2e3c59e

github-actions Bot added the core label Apr 2, 2026

rahulsmahadev added 2 commits April 2, 2026 01:20

fix style

5c2b05f

fix checkstyle: add hasMessage check to assertThatThrownBy

b266f27

Co-authored-by: Isaac

amogh-jahagirdar approved these changes Apr 2, 2026

View reviewed changes

amogh-jahagirdar requested review from danielcweeks and singhpk234 April 2, 2026 19:30

Address Amogh's comments

68120fb

github-actions Bot added the API label Apr 2, 2026

rahulsmahadev changed the title ~~Core: Make REST scan planning poll timeout configurable~~ API, Core: Make REST scan planning poll timeout configurable Apr 2, 2026

singhpk234 reviewed Apr 3, 2026

View reviewed changes

amogh-jahagirdar self-requested a review April 3, 2026 02:52

address comments

4db0c11

github-actions Bot removed the API label Apr 3, 2026

rahulsmahadev changed the title ~~API, Core: Make REST scan planning poll timeout configurable~~ Core : Make REST scan planning poll timeout configurable Apr 3, 2026

rahulsmahadev requested a review from singhpk234 April 3, 2026 21:31

amogh-jahagirdar approved these changes Apr 6, 2026

View reviewed changes

singhpk234 approved these changes Apr 6, 2026

View reviewed changes

amogh-jahagirdar merged commit e1a2713 into apache:main Apr 7, 2026
36 checks passed

		* <p>Clients may catch this to fall back to client-side planning when server-side planning is too
		* slow.

Conversation

rahulsmahadev commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

amogh-jahagirdar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Apr 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rahulsmahadev commented Apr 2, 2026 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar Apr 2, 2026 •

edited

Loading

danielcweeks Apr 2, 2026 •

edited

Loading

amogh-jahagirdar Apr 2, 2026 •

edited

Loading

amogh-jahagirdar Apr 3, 2026 •

edited

Loading

amogh-jahagirdar Apr 3, 2026 •

edited

Loading