Hi Stephen, I think there are two parts to this, and I may have responded to the wrong part.
I can easily and cleanly make those subclasses implement AutoCloseable and remove that from FlatMapStep. Implementing AutoCloseable is needed for the second half of the problem, where a traversal is closed before it is read to completion. This is easily delegated to the subclasses. There's the first half of the problem dealing with closing iterators as FlatMapStep finishes with them, but before it loses its reference to them. This is very hard to remove from FlatMapStep without duplicating all the code in the subclasses. If you consent I'd like to update the pull request that removes AutoCloseable from FlatMapStep but keeps the closing of iterators on the fly in FlatMapStep. I'd can also move that try/catch stuff to a helper method (in CloseableInterface?). That keeps FlatMapStep very clean. Thoughts? public abstract class FlatMapStep<S, E> extends AbstractStep<S, E> { private Traverser.Admin<S> head = null; protected Iterator<E> iterator = EmptyIterator.instance(); // Made protected public FlatMapStep(final Traversal.Admin traversal) { super(traversal); } @Override protected Traverser.Admin<E> processNextStart() { while (true) { if (this.iterator.hasNext()) { return this.head.split(this.iterator.next(), this); } else { this.head = this.starts.next(); closeIterator(); // Added this.iterator = this.flatMap(this.head); } } } protected abstract Iterator<E> flatMap(final Traverser.Admin<S> traverser); @Override public void reset() { super.reset(); closeIterator(); // Added this.iterator = EmptyIterator.instance(); } void closeIterator() { CloseableIterator.closeIterator(this.iterator); } } Thanks, -Paul -----Original Message----- From: Paul A. Jackson [mailto:paul.jack...@pb.com] Sent: Friday, January 27, 2017 1:55 PM To: dev@tinkerpop.apache.org Subject: RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator I can do this, and I can see why you are suggesting it, but I didn't originally because it means overriding all the methods from FlatMapStep into these subclasses. The way it's currently designed all the subclasses need to do is implement flatMap(). FlatMapStep has private access to iterator and head. It knows when it is done with an iterator, that when it's done with an iterator that it should replace it with an EmptyIterator, that if it's not done it needs to call head.split with the next element, etc. To implement what you suggest, correct me if I'm wrong, all that private knowledge needs to be duplicated in the subclasses. If feels like a bad design because if any of those behaviors change in FlatMapStep we need to remember to go into the subclasses and mirror the changes there. Maybe I should move the try/catch stuff to a helper method so that all FlatMapStep needs to do is execute one line when it's done with an iterator? Should I submit parallel pull request so you can compare/contrast? -Paul -----Original Message----- From: spmallette [mailto:g...@git.apache.org] Sent: Friday, January 27, 2017 1:23 PM To: dev@tinkerpop.apache.org Subject: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/548 I'm pretty sure @okram was affirming that `VertexStep`, `EdgeVertexStep` and `PropertiesStep` should implement `AutoCloseable` (I don't think that there are others) rather than a blanket change of just applying it to `FlatMapStep`. As the logic is re-used, perhaps you could do an `ElementStep` that extends the `FlatMapStep` and implements `AutoCloseable`. Then have those three steps extend `ElementStep` which would contain the close logic that you have. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- ________________________________ ________________________________