Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TINKERPOP-1589 Re-introduced CloseableIterator #521

Merged
merged 2 commits into from
Jan 9, 2017
Merged

Conversation

spmallette
Copy link
Contributor

https://issues.apache.org/jira/browse/TINKERPOP-1589

Made it so that the CloseableIterator is closed by GraphStep if it is provided by the iterator supplier. Furthermore, any steps in a Traversal implement AutoCloseable then its close() method will be called. In this way Traversal.close(), the common API a user will work with, has a way to release resources in Graph implementations that require it.

Builds with docker/build.sh -t - i -n.

Made it so that the CloseableIterator is closed by GraphStep if it is provided by the iterator supplier. Furthermore, any steps in a Traversal implement AutoCloseable then its close() method will be called.
@okram
Copy link
Contributor

okram commented Jan 5, 2017

Nice. Clean backwards compatible GraphStep.close() method which uses instanceof.

Are you looking for a VOTE now or are you still building on this PR? -- e.g. integrating it with Traversal.close()/etc.

@spmallette
Copy link
Contributor Author

I think I'm done - It's already implemented on Traversal.close(). I think it's good for vote. Don't think i'm missing anything.

@@ -254,9 +254,14 @@ public default void forEachRemaining(final Consumer<? super E> action) {
}
}

/**
* Releases resources opened in any steps that implement {@link AutoCloseable}.
*/
@Override
public default void close() throws Exception {
Copy link
Contributor

@okram okram Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad. Steps that are TraversalParents are responsible for propagating operations on them to those below. I would do the following:

// Traversal.close()
for(final Step<?,?> step : this.getSteps()) {
  if(step instanceof AutoCloseable)
    step.close();
}

Next, I would have TraversalParent extend AutoCloseable with its default close() method being:

// TraversalParent.close()
for(final Traversal.Admin<?,?> traversal : this.getLocalChildren()) {
  traversal.close()
}

for(final Traversal.Admin<?,? traversal: this.getGlobalChildren()) {
  traversal.close();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes are made now - thanks - didn't know that stuff.

Implemented AutoCloseable on TraversalParent.
@okram
Copy link
Contributor

okram commented Jan 5, 2017

VOTE +1

1 similar comment
@lvca
Copy link
Contributor

lvca commented Jan 5, 2017

VOTE +1

@twilmes
Copy link
Contributor

twilmes commented Jan 5, 2017

VOTE: +1

@asfgit asfgit merged commit 67e0b37 into tp32 Jan 9, 2017
@asfgit asfgit deleted the TINKERPOP-1589 branch February 21, 2017 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants