[GitHub] [iceberg] kbendick commented on a diff in pull request #4863: Fix invalid Stream to Iterable conversion in Tasks

[GitBox <gi...@apache.org>]

kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r882979586


##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -88,8 +89,17 @@ public UnrecoverableException(Throwable cause) {
     private long maxSleepTimeMs = 600000; // 10 minutes
     private long maxDurationMs = 600000;  // 10 minutes
     private double scaleFactor = 2.0;     // exponential
+    private boolean hasStarted = false;
 
+    /**
+     * @deprecated Use {@link #Builder(Iterator)} instead.

Review Comment:
   Nit: normally we add `@deprecated since <version>[, will be removed in <version>]; Use ....`.
   
   I think you can use `@deprecated since 0.14.0`. As to when / if we remove this, I wouldn't worry about it too much (as I'm not sure). 



##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -205,11 +215,12 @@ private <E extends Exception> boolean runSingleThreaded(
       List<I> succeeded = Lists.newArrayList();
       List<Throwable> exceptions = Lists.newArrayList();
 
-      Iterator<I> iterator = items.iterator();
+      Preconditions.checkState(!hasStarted, "Cannot run twice");
+      hasStarted = true;

Review Comment:
   Nit: normally we only use `this.` for field when mutating them. In most cases, this winds up just being the constructor or when set in the builder.
   
   I'm not sure if we would use it here or not.
   
   I wouldn't block on this by any means, but figured I'd mention it 🙂 



##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -88,8 +89,17 @@ public UnrecoverableException(Throwable cause) {
     private long maxSleepTimeMs = 600000; // 10 minutes
     private long maxDurationMs = 600000;  // 10 minutes
     private double scaleFactor = 2.0;     // exponential
+    private boolean hasStarted = false;

Review Comment:
   Since this can be updated in `runParallel`, should we consider making this an `AtomicBoolean` like the booleans defined inside of `runParallel` are?



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