You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/25 10:48:07 UTC

[GitHub] [iceberg] findepi opened a new pull request, #4863: Fix invalid Stream to Iterable conversion in Tasks

findepi opened a new pull request, #4863:
URL: https://github.com/apache/iceberg/pull/4863

   `Stream.iterator` is a one-shot operation, cannot be called multiple
   times, so `stream::iterator` shouldn't be used as an implementation of
   `Iterable`.
   
   Similar to https://github.com/apache/iceberg/pull/4806 


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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#issuecomment-1139426379

   Thank you @kbendick for your review!


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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r885269683


##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -570,7 +584,7 @@ public static <I> Builder<I> foreach(I... items) {
   }
 
   public static <I> Builder<I> foreach(Stream<I> items) {
-    return new Builder<>(items::iterator);
+    return new Builder<>(items.iterator());

Review Comment:
   > No, `stream::iterator` will allow calling `iterator` more than once.
   
   I didn't know that, thanks!
   
   > Adding a single-use iterable would fix this 
   
   That's true. But `Iterable` interface contract isn't clear about whether "a single-use iterable" is a valid implementation, so I wouldn't want to create such a class. I wouldn't stop you if you wanted to, though. 
   
   > without lots of code changes. 
   
   from long-term design perspective this may or may not be a dominant factor



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


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

Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r885054622


##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -570,7 +584,7 @@ public static <I> Builder<I> foreach(I... items) {
   }
 
   public static <I> Builder<I> foreach(Stream<I> items) {
-    return new Builder<>(items::iterator);
+    return new Builder<>(items.iterator());

Review Comment:
   No, `stream::iterator` will allow calling `iterator` more than once. Adding a single-use iterable would fix this without lots of code changes.



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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323642


##########
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:
   It doesn’t have to be added if it truly doesn’t add values.
   
   We have some places where it’s not present, but I also tend to think of it as a notice for people who fork the project in case they’re using it.
   
   I also think of it as something that reminds us (or specifically release managers) when something can be removed entirely.
   
   But I agree here it wouldn’t present much end user facing value. Mostly this comment was just to point out something that is standard practice but not enough so to be automated / forced (due to situations like this).



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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323642


##########
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:
   It doesn’t have to be added if it truly doesn’t add values.
   
   We have some places where it’s not present, but I also tend to think of it as a notice for people who fork the project in case they’re using it.
   
   I also think of it as something that reminds us (or specifically release managers) when something can be removed entirely.
   
   Burr I agree here it wouldn’t present much end user facing value.



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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r885025072


##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -570,7 +584,7 @@ public static <I> Builder<I> foreach(I... items) {
   }
 
   public static <I> Builder<I> foreach(Stream<I> items) {
-    return new Builder<>(items::iterator);
+    return new Builder<>(items.iterator());

Review Comment:
   > I think a better fix is to introduce a one-time use `Iterable` that enforces `hasStarted` internally.
   
   i would assume `stream::iterator` does that. Still, it's not a real `Iterable` IMO.



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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r883421299


##########
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:
   `runParallel` starts threads, but i agree with you, the field isn't mutated in such a thread. 



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


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

Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323165


##########
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:
   Only use `this.` when modifying instance state. It helps the reader know that it is instance state being modified rather than local state. No need to use it when accessing state, only when modifying.



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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884324269


##########
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:
   Isn’t this line of code modifying instances state though (assigning a class variable from false to true)?
   
   EDIT - I see what you’re clarifying. Never mind.



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


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

Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323776


##########
core/src/main/java/org/apache/iceberg/util/Tasks.java:
##########
@@ -570,7 +584,7 @@ public static <I> Builder<I> foreach(I... items) {
   }
 
   public static <I> Builder<I> foreach(Stream<I> items) {
-    return new Builder<>(items::iterator);
+    return new Builder<>(items.iterator());

Review Comment:
   I think a better fix is to introduce a one-time use `Iterable` that enforces `hasStarted` internally. That way we don't need to modify the code in `Tasks`, which is a much larger risk.



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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r883420763


##########
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:
   Should i also reference `hasStarted` with `this.` in the checkState above?
   
   should i also reference `items` with `this.` when calling `.next()`? (this is a mutation after all)
   
   Please advise what should i do.



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


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

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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323642


##########
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:
   It doesn’t have to be added if it truly doesn’t add values.
   
   We have some places where it’s not present, but I also tend to think of it as a notice for people who fork the project in case they’re using it.
   
   I also think of it as something that reminds us (or specifically release managers) when something can be removed entirely. Though we’d need to remove usages first.
   
   But I agree here it wouldn’t present much end user facing value. Mostly this comment was just to point out something that is standard practice but not enough so to be automated / forced (due to situations like this).



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


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

Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884323298


##########
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.
+     */
+    @Deprecated

Review Comment:
   There's no need to deprecate this. It isn't invalid to pass an `Iterable`.



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


[GitHub] [iceberg] findepi closed pull request #4863: Fix invalid Stream to Iterable conversion in Tasks

Posted by GitBox <gi...@apache.org>.
findepi closed pull request #4863: Fix invalid Stream to Iterable conversion in Tasks
URL: https://github.com/apache/iceberg/pull/4863


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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r883139748


##########
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:
   Looking closer, it seems like this wouldn't actually be set in some other thread, so I suppose it's ok. I would defer to your expertise on the issue 🙂 



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


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

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r884324269


##########
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:
   Isn’t this line of code modifying instances state though (assigning a class variable from false to true)?



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


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

Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4863:
URL: https://github.com/apache/iceberg/pull/4863#discussion_r883419844


##########
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:
   > I think you can use `@deprecated since 0.14.0`.
   
   I don't think it adds value. If a user is < 0.14, they won't see deprecation notice.
   If they are on >= 0.14, they see it. 
   
   It would be valuable if it comes with the "will be removed", but it's not my call to make promises like this.
   
   Please advise what should i do.



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