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:[email protected]]
Sent: Friday, January 27, 2017 1:55 PM
To: [email protected]
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:[email protected]]
Sent: Friday, January 27, 2017 1:23 PM
To: [email protected]
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 [email protected] or file a JIRA ticket with
INFRA.
---
________________________________
________________________________
