amogh-jahagirdar commented on code in PR #14629:
URL: https://github.com/apache/iceberg/pull/14629#discussion_r2542613390
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -777,6 +774,9 @@ private static void planFilesFor(TableScan tableScan,
String planId, int tasksPe
private static void asyncPlanFiles(
TableScan tableScan, String asyncPlanId, int tasksPerPlanTask) {
- ASYNC_PLANNING_POOL.execute(() -> planFilesFor(tableScan, asyncPlanId,
tasksPerPlanTask));
+ // Its not necessary to run this in a separate thread pool, but doing so
+ // will create a race condition where a client can call fetchPlanningResult
+ // even before the IN_MEMORY_PLANNING_STATE has been populated.
+ planFilesFor(tableScan, asyncPlanId, tasksPerPlanTask);
Review Comment:
I guess I don't quite understand this comment:
```
// will create a race condition where a client can call fetchPlanningResult
// even before the IN_MEMORY_PLANNING_STATE has been populated.
```
This would mean that the server should still return `submitted` on the
fetchPlanningResult? I think that's where the real fix is, asyncPlanning
essentially needs to keep track of "ongoing" and then remove it when it's
completed, and the server handling makes sure it checks those states
accordingly when trying to return the status. Right now, I could see that
there's a race.
But I'm also OK if we just want to start simple for testing and sort of
"fake" the async, I just thought it'd be a more realistic way to implement it.
--
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]
