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 2021/07/28 06:01:16 UTC
[GitHub] [iceberg] saurabhagas opened a new pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
saurabhagas opened a new pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881
--
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] rymurr commented on pull request #2881: API: Close the underlying iterator in ClosingIterator correctly.
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#issuecomment-889944454
ahh, thanks for the check @openinx you are absolutely right. I will be more cautious next time.
--
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] vvellanki commented on pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
vvellanki commented on pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#issuecomment-888069862
@rymurr Can you review this patch?
--
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] rymurr commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by 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
[GitHub] [iceberg] rymurr commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678976219
##########
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:
What is the behaviour if htis iterator is being used in parallel?
--
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] rymurr merged pull request #2881: API: Close the underlying iterator in ClosingIterator correctly.
Posted by GitBox <gi...@apache.org>.
rymurr merged pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r679427455
##########
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:
The iterator isn't required to be thread-safe from what I can tell.
--
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] nastra commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678049808
##########
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:
you probably should make sure that `close()` is only called once
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678060762
##########
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:
`Closeable::close` is required to be idempotent - if calling close on a `Closeable` resource has any side-effects, we should fix that at the source. This would also mean that calling close() multiple times should not throw any exceptions.
--
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] openinx commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678048412
##########
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:
I think you mean in the read pattern (for the original `ClosingIterator`)
```java
while (hasNext()) {
T val = next();
}
```
When the hasNext() return false, then we won't call the `next()` to close the internal `CloseableIterator`. Yes, I think you are correct, but I think we will need to guarantee that we will only close the `CloseableIterator` once even if we called the `ClosingIterator#hasNext` many times, otherwise once we've called the first `hasNext`, all the following `hasNext` check will be thrown an exception, that's not the expected behavior.
--
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] saurabhagas commented on pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#issuecomment-889771040
Thanks @rymurr and other reviewers for accepting the change.
--
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 change in pull request #2881: API: Close the underlying iterator in ClosingIterator correctly.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r680099221
##########
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:
I think this was an oversight. I don't see a reason why it shouldn't be `Closeable`.
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r679426943
##########
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:
If a premature close is required, the underlying `CloseableIterator` should be used directly. I see this class as a convenience wrapper for the case when all of the elements are intended to be consumed. I've added Javadoc mentioning 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] openinx commented on pull request #2881: API: Close the underlying iterator in ClosingIterator correctly.
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#issuecomment-889902475
@rymurr , I think we will need to check whether the contributor are following the commit message pattern: `[iceberg-module]: [commit log message]`. For example, this commit message should be rewrite as `API: Close the underlying iterator in ClosingIterator correctly`. If the contributor did not follow the pattern, we committer will need to change it to (I think it's also good to add this to the apache iceberg [document](https://iceberg.apache.org/community/#contributing)). We don't need to revert this committed PR, please have an extra check next time :-) .
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678161827
##########
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:
Done, and added a test.
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678161635
##########
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:
Done, and added a test.
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678059156
##########
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:
> you probably should make sure that `close()` is only called once
`Closeable::close` is required to be idempotent - if calling close on a `Closeable` resource has any side-effects, we should fix that at the source.
--
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] rymurr commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678974726
##########
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:
What about the case when the developer doesn't consume the entire iterator but has scheduled this to be closed?
--
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] openinx commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678067469
##########
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 should fix that at the source
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. The correct approach is keeping it close once in this `ClosingIterator`. I think introducing a boolean `closed` is enough ?
--
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] saurabhagas commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call
Posted by GitBox <gi...@apache.org>.
saurabhagas commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678163170
##########
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:
I think this is by design. The intent must be to avoid leaks because the developer forgot to call `close()` on the iterator.
--
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