Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/159#discussion_r108475855
--- Diff:
core/src/main/java/org/apache/accumulo/core/iterators/SortedKeyValueIterator.java
---
@@ -147,4 +147,21 @@
* if not supported.
*/
SortedKeyValueIterator<K,V> deepCopy(IteratorEnvironment env);
+
+ /**
+ * Closes any resources that were opened by the
<tt>SortedKeyValueIterator</tt>. This method does nothing by default. The
implementing class must close its
+ * <tt>SortedKeyValueIterator source</tt>. This will be provided in the
<tt>init</tt> method or by the constructor.
+ *
+ * @exception UncheckedIOException
--- End diff --
I've been really mulling over whether or not to bring this up, but
whatever, too late now:
Is throwing `UncheckedIOException` harmful to us? I feel like this will
make our server-side code more brittle (forgetting that we actually need to
handle unexpected runtime situations) for the (small) gain of being able to use
`AutoCloseable` (which I see as nothing but syntactic sugar). @dhutchis also
touched on this, recommending that `close()` be idempotent, moving from
`AutoCloseable` to just `Closeable`.
For example, what if I had an Iterator which was buffering some state, when
the iterator was closed, it would flush this data to some external location
(read as: "performs I/O on `close()`"). We have no way to handle this scenario
cleanly (users wouldn't even *know* it happened).
Is there some benefit of being `AutoCloseable` that I'm not realizing? Or,
am I blowing this concern out of scope of what this change is really meant to
do (pointing that we need more docs as to what `close()` is limited to support)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
