[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-10-17 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 @ctubbsii @joshelser @keith-turner I made changes for previous discussions in e701ab6. The biggest impact occurred in RFile and FileUtil where I modified the exception handling. FileUtil shou

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-10-25 Thread keith-turner
Github user keith-turner commented on the issue: https://github.com/apache/accumulo/pull/159 It would be really nice to have unit test for all of the iterators that were changed to make sure they close their source. --- If your project is set up for it, you can reply to this email an

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-14 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 With the exception of OrIterator and Keith's concerns about Scanner reusing RFiles, I believe I have addressed all comments. I created ACCUMULO-4520 for implementation of Christopher's ideas p

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-15 Thread keith-turner
Github user keith-turner commented on the issue: https://github.com/apache/accumulo/pull/159 I tried running mvn verify -Psunny and the ReadWriteIT failed. Scans were failing because of server side errors. Seeing lots exceptions like the following in the tablet server logs. I suspe

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-16 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 After discussing the issues with these changes with @keith-turner we decided to narrow the scope of the close implementation to only be for User Iterators. My attempt at trying to close iterat

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-16 Thread joshelser
Github user joshelser commented on the issue: https://github.com/apache/accumulo/pull/159 > we decided to narrow the scope of the close implementation to only be for User Iterators How are "user iterators" going to be defined? --- If your project is set up for it, you can re

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-16 Thread keith-turner
Github user keith-turner commented on the issue: https://github.com/apache/accumulo/pull/159 More specifically, we discussed not closing RFiles and in mem map. One easy way to do this is to make the MultiIterator close do nothing. System and user iters sit above the multi-iterator.

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-17 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 I reverted a few changes with 92e6def901ea868d84f40d282275bce585a2db05 and mvn verify -Psunny now passes. I think the remaining files in question are Scanner and Compactor. I am writing a tes

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-17 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 I ran "mvn verify -Psunny" and all tests passed. I am running the full "mvn verify" now on AWS. --- If your project is set up for it, you can reply to this email and have your reply appear o

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-18 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Full "mvn verify" had errors in accumulo-test : Tests in error: BigRootTabletIT.test:58 » Accumulo org.apache.thrift.TApplicationException: In... LargeRowIT.run:125->test2:142

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-18 Thread keith-turner
Github user keith-turner commented on the issue: https://github.com/apache/accumulo/pull/159 The `internal error` needs to be investigated. This indicates an unexpected error in the tablet server. Can look in the tserver logs for that test. --- If your project is set up for it, yo

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-18 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 I ran WriteAheadLogIT by itself and it passed. It looks like my VM just ran out of disk space. During the BigRootTabletIT... the RFile is being closed by SSI during switchSource. Her

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-18 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Here is my full debug stacktrace where RFile is being closed: ``` java.lang.Exception at org.apache.accumulo.core.file.rfile.RFile$LocalityGroupReader.close(RFile.java:714)

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-18 Thread dhutchis
Github user dhutchis commented on the issue: https://github.com/apache/accumulo/pull/159 Bringing the discussion on exception handling back to the top level, and adding more thoughts. ## Exceptions and Closing Suppose you're an iterator. You call `next()` or `seek()`

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-21 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Thanks for the input @dhutchis. > Specify the contract that every iterator must close() its source. This strategy works if every iterator, system and user-defined, follows the contra

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-21 Thread dhutchis
Github user dhutchis commented on the issue: https://github.com/apache/accumulo/pull/159 > The problem I am running into is our underlying reuse of RFiles throughout the code. One solution you suggested is to make the RFile close() methods do nothing. (Rename the current clos

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-21 Thread ctubbsii
Github user ctubbsii commented on the issue: https://github.com/apache/accumulo/pull/159 @dhutchis wrote: > With this approach, each iterator is only responsible for closing the resources it itself opens. No need to consider other iterators. Seems more palatable, don't you think?

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-21 Thread dhutchis
Github user dhutchis commented on the issue: https://github.com/apache/accumulo/pull/159 > That approach won't work with deep copies, or similar scenarios where the iterator "stack" grows outside the purview of the framework. Good point =) --- If your project is set up for i

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-22 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 After some discussion yesterday with @ctubbsii we came up with a compromise, that I believe will benefit everyone. I can document the interface to follow the design that states "All Iterators

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2016-11-23 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Another attempt at the proper language for the interface: 1a6cfca3e81a63d410326b32f25fd148fbede9e3 --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2017-03-27 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Revisiting these changes... rebased and running verify -Psunny. If all goes well I will merge these changes soon. --- If your project is set up for it, you can reply to this email and have y

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2017-03-28 Thread joshelser
Github user joshelser commented on the issue: https://github.com/apache/accumulo/pull/159 > If all goes well I will merge these changes soon. I don't see any doc-related changes included... --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2017-03-28 Thread milleruntime
Github user milleruntime commented on the issue: https://github.com/apache/accumulo/pull/159 Added entry to the Iterator design section of the user manual in 752c12e --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If you

[GitHub] accumulo issue #159: ACCUMULO-1280: many changes for closing iterators

2017-03-28 Thread joshelser
Github user joshelser commented on the issue: https://github.com/apache/accumulo/pull/159 > > we decided to narrow the scope of the close implementation to only be for User Iterators >How are "user iterators" going to be defined? Ping on this one as I think it got lo