kbendick commented on a change in pull request #1446:
URL: https://github.com/apache/iceberg/pull/1446#discussion_r487814087
##########
File path: core/src/main/java/org/apache/iceberg/util/ParallelIterable.java
##########
@@ -98,6 +99,22 @@ private boolean checkTasks() {
for (int i = 0; i < taskFutures.length; i += 1) {
if (taskFutures[i] == null || taskFutures[i].isDone()) {
+ if (taskFutures[i] != null) {
+ // check for task failure and re-throw any exception
+ try {
+ taskFutures[i].get();
Review comment:
So I see that there are multple places where ParallelIterator class is
`synchronized` or otherwise blocking. However, from my reading of the rest of
the class at least, it seems that we can submit two tasks per worker.
Is there any way we can avoid blocking on futures indefinitely in a loop,
like for example possibly somehow joining the currently running futures and
then throwing as sooon as one of them throws an exception. Perhaps there's
something I'm missing that makes this less of a concern than it seems
initially. If that's the case, could we possibly add a comment indicating why
it's not a concern?
I'm just thinking out loud here, but to me, Future.get usually feels like a
code smell.
Though I'm supportive if this will fix flaky tests. And if there's no
concern, then that's great. If there is concern, my feeling is that maybe it's
best to fix the flaky tests and then open an issue to be sure to come back to
this to have `ParallelIterable` be more... well, parallel. But again, I might
be missing something that makes it more parallel than it seems to me at first
glance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
