[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/548 VOTE: +1 --- 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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/548 All tests pass with `docker/build.sh -t -n -i` VOTE +1 --- 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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/548 VOTE +1. --- 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. ---
Re: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
yeah - github picks up your changes on the branch automatically. we'll work
on reviewing.
On Fri, Jan 27, 2017 at 7:11 PM, Paul A. Jackson <paul.jack...@pb.com>
wrote:
> I am git illiterate.
> I committed to my repo.
> It looks like that's all I have to do.
> Travis build tells me to hang tight.
> It knows the commit is related to an open pull request
> and your repo knows to pull it in automatically?
> That's cool, if true.
> -Paul
>
> -Original Message-
> From: Paul A. Jackson [mailto:paul.jack...@pb.com]
> Sent: Friday, January 27, 2017 2:24 PM
> To: dev@tinkerpop.apache.org
> Subject: RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced
> CloseableIterator
>
> 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 head = null;
> protected Iterator iterator = EmptyIterator.instance(); // Made
> protected
>
> public FlatMapStep(final Traversal.Admin traversal) {
> super(traversal);
> }
>
> @Override
> protected Traverser.Admin 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 flatMap(final Traverser.Admin
> 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 t
RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
I am git illiterate.
I committed to my repo.
It looks like that's all I have to do.
Travis build tells me to hang tight.
It knows the commit is related to an open pull request
and your repo knows to pull it in automatically?
That's cool, if true.
-Paul
-Original Message-
From: Paul A. Jackson [mailto:paul.jack...@pb.com]
Sent: Friday, January 27, 2017 2:24 PM
To: dev@tinkerpop.apache.org
Subject: RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced
CloseableIterator
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 head = null;
protected Iterator iterator = EmptyIterator.instance(); // Made protected
public FlatMapStep(final Traversal.Admin traversal) {
super(traversal);
}
@Override
protected Traverser.Admin 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 flatMap(final Traverser.Admin 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.
---
RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
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 head = null;
protected Iterator iterator = EmptyIterator.instance(); // Made protected
public FlatMapStep(final Traversal.Admin traversal) {
super(traversal);
}
@Override
protected Traverser.Admin 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 flatMap(final Traverser.Admin 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.
---
[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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/548 ha - "makes sense" that the way it is now is ok or "makes sense" as with the change i proposed? :smile: --- 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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/548 Yes. That makes sense to me. --- 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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/548 tests all pass with docker so that's good. @okram would you prefer to not see this change made to `FlatMapStep` and instead see it implemented specifically in steps that interact with the Structure API, like: `VertexStep`? or is this ok from your perspective? --- 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. ---
[GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced CloseableIterator
Github user pauljackson commented on the issue: https://github.com/apache/tinkerpop/pull/548 Hadoop build is failing for me, but this happens under normal conditions. --- 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. ---
