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 2020/05/07 13:16:19 UTC
[GitHub] [incubator-iceberg] openinx opened a new pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
openinx opened a new pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011
When I read the BaseDataReader.java, I find that there's a member named currentCloseable which would be set with a value in its sub-class (RowDataReader), that confuse me a lot. This also introduced a checkstyle waring `@SuppressWarnings("checkstyle:VisibilityModifier")`.
I think it will be more clear if we introduce a CloseableIterator, say for each CloseableIterable we can create a CloseableIterator, once we iterated all the element then just call the iterator#close to close the resources, don't have to maintain two instances: one for Iterator and one for 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.
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] [incubator-iceberg] rdblue commented on a change in pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#discussion_r421616138
##########
File path: api/src/main/java/org/apache/iceberg/io/CloseableIterable.java
##########
@@ -32,6 +32,27 @@
import org.apache.iceberg.exceptions.RuntimeIOException;
public interface CloseableIterable<T> extends Iterable<T>, Closeable {
+
+ default CloseableIterator<T> closeableIterator() {
+ Iterator<T> iterator = iterator();
+ return new CloseableIterator<T>() {
+ @Override
+ public void close() throws IOException {
+ CloseableIterable.this.close();
Review comment:
This isn't correct. `CloseableIterable#close` will close all open iterators created by the `Iterable`. The purpose is to enable try-with-resources instead of relying on callers to remember to close:
```java
try (CloseableIterable<T> iterable = open(...)) {
for (T item : iterable) {
process(item);
}
}
```
If an iterator closed the iterable, then it would close other iterators that may be still used in other threads.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] openinx commented on pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#issuecomment-626437642
Ping @rdblue, How do you think about the patch ? Is it worth to get this merge in trunk ? If so please take a look, otherwise I will close it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#issuecomment-629339679
Merged. Thanks for fixing 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.
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] [incubator-iceberg] openinx commented on pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#issuecomment-629166982
@rdblue , Thanks for the remanding, I've fixed the conflicts in commit: https://github.com/apache/incubator-iceberg/pull/1011/commits/132afe5c3d59c2146254289e5f76fecb828017a1
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#discussion_r421616138
##########
File path: api/src/main/java/org/apache/iceberg/io/CloseableIterable.java
##########
@@ -32,6 +32,27 @@
import org.apache.iceberg.exceptions.RuntimeIOException;
public interface CloseableIterable<T> extends Iterable<T>, Closeable {
+
+ default CloseableIterator<T> closeableIterator() {
+ Iterator<T> iterator = iterator();
+ return new CloseableIterator<T>() {
+ @Override
+ public void close() throws IOException {
+ CloseableIterable.this.close();
Review comment:
This isn't correct. `CloseableIterable#close` will close all open iterators created by the `Iterable`. The purpose is to enable try-with-resources instead of relying on callers to remember to close:
```java
try (CloseableIterable<T> iterable = open(...)) {
for (T item : iterable) {
process(item);
}
}
```
If an iterator closed the iterable, then it would close other iterators that may be still used in other threads.
Instead, this should check whether `iterator` is `Closeable` and call close on it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#discussion_r421617642
##########
File path: mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java
##########
@@ -359,25 +357,26 @@ public void close() throws IOException {
return nameToPos;
}
- private Iterator<T> open(FileScanTask currentTask) {
+ private CloseableIterator<T> open(FileScanTask currentTask) {
DataFile file = currentTask.file();
// schema of rows returned by readers
PartitionSpec spec = currentTask.spec();
- Set<Integer> idColumns = Sets.intersection(spec.identitySourceIds(), TypeUtil.getProjectedIds(expectedSchema));
+ Set<Integer> idColumns = Sets.intersection(spec.identitySourceIds(), TypeUtil.getProjectedIds(expectedSchema));
Review comment:
Even if this is correct style, I don't think it is worth changing because it can cause commit conflicts.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#discussion_r425426417
##########
File path: mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java
##########
@@ -396,8 +395,7 @@ public void close() throws IOException {
throw new UnsupportedOperationException(
String.format("Cannot read %s file: %s", file.format().name(), file.path()));
}
- currentCloseable = iterable;
- return iterable.iterator();
+ return iterable;
Review comment:
We follow the general style of Hadoop projects that I think is based on a Google style doc somewhere. We also add the rules to checkstyle when it can enforce them.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#issuecomment-628883732
@openinx, I think this is ready to go, but it has conflicts. Could you fix those and I'll merge?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue merged pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] openinx commented on a change in pull request #1011: Introduce a CloseableIterator to remove the consfusing currentCloseable in BaseDataReader
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #1011:
URL: https://github.com/apache/incubator-iceberg/pull/1011#discussion_r421918261
##########
File path: api/src/main/java/org/apache/iceberg/io/CloseableIterable.java
##########
@@ -32,6 +32,27 @@
import org.apache.iceberg.exceptions.RuntimeIOException;
public interface CloseableIterable<T> extends Iterable<T>, Closeable {
+
+ default CloseableIterator<T> closeableIterator() {
+ Iterator<T> iterator = iterator();
+ return new CloseableIterator<T>() {
+ @Override
+ public void close() throws IOException {
+ CloseableIterable.this.close();
Review comment:
Thanks for pointing this out, I've fixed this in the new 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.
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