[GitHub] [iceberg] rymurr commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call

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

rymurr commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678079932



##########
File path: api/src/main/java/org/apache/iceberg/io/ClosingIterator.java
##########
@@ -25,33 +25,30 @@
 
 public class ClosingIterator<T> implements Iterator<T> {
   private final CloseableIterator<T> iterator;
-  private boolean shouldClose = false;
 
   public ClosingIterator(CloseableIterator<T> iterator) {
     this.iterator = iterator;
-
   }
 
   @Override
   public boolean hasNext() {
     boolean hasNext = iterator.hasNext();
-    this.shouldClose = !hasNext;
+    if (!hasNext) {
+      close();

Review comment:
       > We can not assume that all the source could guarantee it only close one times, even if all the resources introduced in apache iceberg project don't guarantee it.
   
   I agree

##########
File path: api/src/main/java/org/apache/iceberg/io/ClosingIterator.java
##########
@@ -25,33 +25,30 @@
 
 public class ClosingIterator<T> implements Iterator<T> {
   private final CloseableIterator<T> iterator;
-  private boolean shouldClose = false;
 
   public ClosingIterator(CloseableIterator<T> iterator) {
     this.iterator = iterator;
-
   }
 
   @Override
   public boolean hasNext() {
     boolean hasNext = iterator.hasNext();
-    this.shouldClose = !hasNext;
+    if (!hasNext) {
+      close();
+    }
     return hasNext;
   }
 
   @Override
   public T next() {
-    T next = iterator.next();
+    return iterator.next();
+  }
 
-    if (shouldClose) {
-      // this will only be called once because iterator.next would throw NoSuchElementException
-      try {
-        iterator.close();
-      } catch (IOException e) {
-        throw new UncheckedIOException(e);
-      }
+  private void close() {

Review comment:
       A cursory search of various closable iterators in the iceberg project indicates that not all are idempotent. I think we should raise a ticket to address the issue in the iceberg project at least. But in the meantime we can just check if this has already been closed.

##########
File path: api/src/main/java/org/apache/iceberg/io/ClosingIterator.java
##########
@@ -25,33 +25,30 @@
 
 public class ClosingIterator<T> implements Iterator<T> {

Review comment:
       @rdblue any reason this is not `Closeable`? The only way for the underlying resource to be closed right now is for this iterator to be exhausted. Should this be `Closable` so the resources can be closed w/o having to read the whole iterator? Or is this by design and we should add javadoc to document this contract?




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