vrozov commented on issue #272: Refactor AvroIterable and prevent multiple iterators URL: https://github.com/apache/incubator-iceberg/pull/272#issuecomment-511462956 I already explained and, I believe, you already agreed that callers 1. won't be able to choose to close an iterator as `Closeable` may be easily lost 1. the code by itself does not imply that there are multiple resources acquired. When a person reads such code, she likely will assume that there is a single resource acquired in try-with-resources. Both cases are examples where logic needs to be modified. For example instead of ```java try (AvroIterable records = Avro.read(...).build()) { for (R record : records) { ... break; } for (R record : records) { ... } } ``` it should be written as ```java List<Record> recordsCached = new ArrayList<>(); try (AvroIterable records = Avro.read(...).build()) { Iterators.addAll(recordsCached, records.iterator()); } for (R record : recordsCached) { ... break; } for (R record : recordsCached) { ... } ``` ability to get multiple iterators promotes less code in favor of less performant code. It should be explicit for callers that they are iterating over a file resource that needs to be closed and that iterating twice over a file is much slower compared to an in-memory collection. Making it implicit will lead to resource leaks and a significant effort in isolating such resource leaks. It is a trade-off for allowing only one active (note additional changes) 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
