REST: Add Idempotency-Key support for scan planning endpoints#15096
REST: Add Idempotency-Key support for scan planning endpoints#15096huaxingao wants to merge 3 commits intoapache:mainfrom
Conversation
| private String planPath(TableIdentifier ident) { | ||
| return String.format( | ||
| "v1/namespaces/%s/tables/%s/plan", | ||
| RESTUtil.encodeNamespace(ident.namespace(), RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8), | ||
| RESTUtil.encodeString(ident.name())); | ||
| } | ||
|
|
||
| private String tasksPath(TableIdentifier ident) { | ||
| return String.format( | ||
| "v1/namespaces/%s/tables/%s/tasks", | ||
| RESTUtil.encodeNamespace(ident.namespace(), RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8), | ||
| RESTUtil.encodeString(ident.name())); | ||
| } | ||
|
|
||
| private String cancelPath(TableIdentifier ident, String planId) { | ||
| return String.format( | ||
| "v1/namespaces/%s/tables/%s/plan/%s", | ||
| RESTUtil.encodeNamespace(ident.namespace(), RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8), | ||
| RESTUtil.encodeString(ident.name()), | ||
| RESTUtil.encodeString(planId)); | ||
| } |
There was a problem hiding this comment.
| FetchScanTasksResponse.class, | ||
| ErrorHandlers.planTaskHandler()); | ||
|
|
||
| // We cancel the planning state before this call, so a *fresh execution* of fetchScanTasks would |
There was a problem hiding this comment.
| // We cancel the planning state before this call, so a *fresh execution* of fetchScanTasks would | |
| // We cancel the planning state before this call, so a fresh execution of fetchScanTasks would |
??
| List<PlanTableScanResponse> responses = | ||
| executeTwice( | ||
| HTTPRequest.HTTPMethod.POST, | ||
| planPath(ident), | ||
| headers, | ||
| request, | ||
| PlanTableScanResponse.class, | ||
| ErrorHandlers.tableErrorHandler()); | ||
| PlanTableScanResponse first = responses.get(0); | ||
| PlanTableScanResponse second = responses.get(1); |
There was a problem hiding this comment.
i am not sure if execute twice is getting something here we any ways are writing 2 loc to get the first and the second invocation ?
There was a problem hiding this comment.
Agreed executeTwice doesn’t really reduce verbosity here. I’ve updated the test to use two explicit execute(...) calls for first and second to make the two invocations unambiguous
| // identifiers (data file locations) to the first response. Use order-insensitive comparison | ||
| // since task ordering isn't guaranteed. |
There was a problem hiding this comment.
why isn't the task ordering gauranteed ?
There was a problem hiding this comment.
In CatalogHandlers.planFilesFor, we iterate scan.planFiles() and directly Iterables.partition(planTasks, tasksPerPlanTask) without sorting, so task order is whatever planFiles() yields and isn’t guaranteed.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2 participants
Follows OpenAPI spec update in #14730 to support Idempotency-Key on mutating scan planning endpoints.