accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From joshelser <...@git.apache.org>
Subject [GitHub] accumulo pull request #159: ACCUMULO-1280: many changes for closing iterator...
Date Tue, 28 Mar 2017 16:53:32 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message