Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: 3.2.3
    • Fix Version/s: 3.2.4
    • Component/s: structure
    • Labels:
      None

      Description

      In Blueprints, we used to have CloseableIterable, which allowed users to release resources that might have been held by the underlying graph database. We didn't port that interface to TinkerPop 3.x for some reason. Graphs like OrientDB require a close() method for iterators returned from methods like Graph.vertices() and Graph.edges(). In addition, allowing close() from those iterators would make the close() on Traversal (non-remote) more useful and meaningful (right now it is an empty method by default).

      In Blueprints, Graph.getVertices() and Graph.getEdges() returned a Iterable and the Graph could choose to return CloseableIterator. Users who wanted to write provider agnostic code would then have to do:

      if (iterable instanceof CloseableIterable))
        ((CloseableIterable) iterable).close();
      

      I'm not sure why we didn't simply return CloseableIterable from those methods. Any reason we shouldn't do that now? Perhaps the approach is to introduce CloseableIterator to 3.2.4, but keep the return type of edges/vertices() as Iterator and then for 3.3.0 change the return type to CloseableIterator. In this way, the feature can be available in next release of 3.2.4 without any API changes and users can do the instanceof check above if they need to. Then for 3.3.0 when we allow API changes we can just make it more convenient with a CloseableIterator return type.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/548

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/548
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/548

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/548 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/548

          VOTE +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/548 VOTE +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pauljackson commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/548#discussion_r98973022

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FlatMapStep.java —
          @@ -44,6 +45,7 @@ public FlatMapStep(final Traversal.Admin traversal)

          { return this.head.split(this.iterator.next(), this); }

          else {
          this.head = this.starts.next();
          + closeIterator();
          — End diff –

          I'll update the PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user pauljackson commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/548#discussion_r98973022 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FlatMapStep.java — @@ -44,6 +45,7 @@ public FlatMapStep(final Traversal.Admin traversal) { return this.head.split(this.iterator.next(), this); } else { this.head = this.starts.next(); + closeIterator(); — End diff – I'll update the PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pauljackson commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/548#discussion_r98972948

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java —
          @@ -164,13 +164,13 @@ public int hashCode() {
          }

          /**

          • * Attemps to close an underlying iterator if it is of type {@link CloseableIterator}. Graph providers may choose
            + * Attempts to close an underlying iterator if it is of type {@link CloseableIterator}

            . Graph providers may choose

          • to return this interface containing their vertices and edges if there are expensive resources that might need to
          • be released at some point.
            */
            @Override
            public void close() throws Exception {
          • if (iterator != null && iterator instanceof CloseableIterator) {
              • End diff –

          `GraphStep` doesn't extend `FlatMapStep`, so it doesn't have access to `closeIterator()`, but it can call `CloseableIterator.closeIterator(iterator)`

          Show
          githubbot ASF GitHub Bot added a comment - Github user pauljackson commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/548#discussion_r98972948 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java — @@ -164,13 +164,13 @@ public int hashCode() { } /** * Attemps to close an underlying iterator if it is of type {@link CloseableIterator}. Graph providers may choose + * Attempts to close an underlying iterator if it is of type {@link CloseableIterator} . Graph providers may choose to return this interface containing their vertices and edges if there are expensive resources that might need to be released at some point. */ @Override public void close() throws Exception { if (iterator != null && iterator instanceof CloseableIterator) { End diff – `GraphStep` doesn't extend `FlatMapStep`, so it doesn't have access to `closeIterator()`, but it can call `CloseableIterator.closeIterator(iterator)`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/548#discussion_r98931140

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java —
          @@ -164,13 +164,13 @@ public int hashCode() {
          }

          /**

          • * Attemps to close an underlying iterator if it is of type {@link CloseableIterator}. Graph providers may choose
            + * Attempts to close an underlying iterator if it is of type {@link CloseableIterator}

            . Graph providers may choose

          • to return this interface containing their vertices and edges if there are expensive resources that might need to
          • be released at some point.
            */
            @Override
            public void close() throws Exception {
          • if (iterator != null && iterator instanceof CloseableIterator) {
              • End diff –

          Why doesn't `GraphStep` implement `AutoCloseable` like the others?

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/548#discussion_r98931140 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java — @@ -164,13 +164,13 @@ public int hashCode() { } /** * Attemps to close an underlying iterator if it is of type {@link CloseableIterator}. Graph providers may choose + * Attempts to close an underlying iterator if it is of type {@link CloseableIterator} . Graph providers may choose to return this interface containing their vertices and edges if there are expensive resources that might need to be released at some point. */ @Override public void close() throws Exception { if (iterator != null && iterator instanceof CloseableIterator) { End diff – Why doesn't `GraphStep` implement `AutoCloseable` like the others?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/548#discussion_r98930854

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FlatMapStep.java —
          @@ -44,6 +45,7 @@ public FlatMapStep(final Traversal.Admin traversal)

          { return this.head.split(this.iterator.next(), this); }

          else {
          this.head = this.starts.next();
          + closeIterator();
          — End diff –

          This should go before `this.starts.next()` as if `this.starts.next()` is empty, it will throw `FastNoSuchElementException` and the previous iterator would not have been closed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/548#discussion_r98930854 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FlatMapStep.java — @@ -44,6 +45,7 @@ public FlatMapStep(final Traversal.Admin traversal) { return this.head.split(this.iterator.next(), this); } else { this.head = this.starts.next(); + closeIterator(); — End diff – This should go before `this.starts.next()` as if `this.starts.next()` is empty, it will throw `FastNoSuchElementException` and the previous iterator would not have been closed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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:

          Show
          githubbot ASF GitHub Bot added a comment - 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:
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/548

          Yes. That makes sense to me.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/548 Yes. That makes sense to me.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user pauljackson opened a pull request:

          https://github.com/apache/tinkerpop/pull/548

          TINKERPOP-1589 Re-introduced CloseableIterator

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

          Add support for the closing of `Iterators` returned from
          `Vertex.vertices()` and `Vertex.edges()`.
          Iterators are closed once they are read to completion.
          Make `FlatMapStep` implement `AutoCloseable` in case iterator is not
          read to completion, should get closed when Traversal is closed.
          Remove unnecessary null check (null is never an instanceof).
          OLTP mode support only. More extensive changes required for OLAP.
          NOTE: Rethrowing checked Exception from `close()` as unchecked
          RuntimeException in order to retain `Step.reset()` and
          `AbstractStep.processNextStart()` signature.
          Fix bug in last commit where iterator is not set to
          EMPTY_ITERATOR for iterators that are not Closeable.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/pauljackson/tinkerpop tp32

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/548.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #548


          commit c8d5ff9db6ec8908abfad434c4fb339c5ad53468
          Author: PaulJackson123 <pauljackson123@verizon.net>
          Date: 2017-01-26T01:43:22Z

          TINKERPOP-1589 Re-introduced CloseableIterator

          https://issues.apache.org/jira/browse/TINKERPOP-1589
          Add support for the closing of Iterators returned from
          Vertex.vertices() and Vertex.edges().
          Iterators are closed once they are read to completion.
          Make FlatMapStep implement AutoCloseable in case iterator is not
          read to completion, should get closed when Traversal is closed.
          Remove unnecessary null check (null is never an instanceof).
          OLTP mode support only. More extensive changes required for OLAP.
          NOTE: Rethrowing checked Exception from close() as unchecked
          RuntimeException in order to retain Step.reset() and
          AbstractStep.processNextStart() signatured.

          commit c2d183acf4418733666d1cd603f15a87beb9aa97
          Author: PaulJackson123 <pauljackson123@verizon.net>
          Date: 2017-01-26T14:52:30Z

          TINKERPOP-1589 Re-introduced CloseableIterator

          https://issues.apache.org/jira/browse/TINKERPOP-1589
          Add support for the closing of Iterators returned from
          Vertex.vertices() and Vertex.edges().
          Iterators are closed once they are read to completion.
          Make FlatMapStep implement AutoCloseable in case iterator is not
          read to completion, should get closed when Traversal is closed.
          Remove unnecessary null check (null is never an instanceof).
          OLTP mode support only. More extensive changes required for OLAP.
          NOTE: Rethrowing checked Exception from close() as unchecked
          RuntimeException in order to retain Step.reset() and
          AbstractStep.processNextStart() signatured.
          Fix bug in last commit where iterator is not set to
          EMPTY_ITERATOR for iterators that are not Closeable.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user pauljackson opened a pull request: https://github.com/apache/tinkerpop/pull/548 TINKERPOP-1589 Re-introduced CloseableIterator https://issues.apache.org/jira/browse/TINKERPOP-1589 Add support for the closing of `Iterators` returned from `Vertex.vertices()` and `Vertex.edges()`. Iterators are closed once they are read to completion. Make `FlatMapStep` implement `AutoCloseable` in case iterator is not read to completion, should get closed when Traversal is closed. Remove unnecessary null check (null is never an instanceof). OLTP mode support only. More extensive changes required for OLAP. NOTE: Rethrowing checked Exception from `close()` as unchecked RuntimeException in order to retain `Step.reset()` and `AbstractStep.processNextStart()` signature. Fix bug in last commit where iterator is not set to EMPTY_ITERATOR for iterators that are not Closeable. You can merge this pull request into a Git repository by running: $ git pull https://github.com/pauljackson/tinkerpop tp32 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/548.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #548 commit c8d5ff9db6ec8908abfad434c4fb339c5ad53468 Author: PaulJackson123 <pauljackson123@verizon.net> Date: 2017-01-26T01:43:22Z TINKERPOP-1589 Re-introduced CloseableIterator https://issues.apache.org/jira/browse/TINKERPOP-1589 Add support for the closing of Iterators returned from Vertex.vertices() and Vertex.edges(). Iterators are closed once they are read to completion. Make FlatMapStep implement AutoCloseable in case iterator is not read to completion, should get closed when Traversal is closed. Remove unnecessary null check (null is never an instanceof). OLTP mode support only. More extensive changes required for OLAP. NOTE: Rethrowing checked Exception from close() as unchecked RuntimeException in order to retain Step.reset() and AbstractStep.processNextStart() signatured. commit c2d183acf4418733666d1cd603f15a87beb9aa97 Author: PaulJackson123 <pauljackson123@verizon.net> Date: 2017-01-26T14:52:30Z TINKERPOP-1589 Re-introduced CloseableIterator https://issues.apache.org/jira/browse/TINKERPOP-1589 Add support for the closing of Iterators returned from Vertex.vertices() and Vertex.edges(). Iterators are closed once they are read to completion. Make FlatMapStep implement AutoCloseable in case iterator is not read to completion, should get closed when Traversal is closed. Remove unnecessary null check (null is never an instanceof). OLTP mode support only. More extensive changes required for OLAP. NOTE: Rethrowing checked Exception from close() as unchecked RuntimeException in order to retain Step.reset() and AbstractStep.processNextStart() signatured. Fix bug in last commit where iterator is not set to EMPTY_ITERATOR for iterators that are not Closeable.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pauljackson closed the pull request at:

          https://github.com/apache/tinkerpop/pull/547

          Show
          githubbot ASF GitHub Bot added a comment - Github user pauljackson closed the pull request at: https://github.com/apache/tinkerpop/pull/547
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pauljackson commented on the issue:

          https://github.com/apache/tinkerpop/pull/547

          Resubmitting this pull request.

          Show
          githubbot ASF GitHub Bot added a comment - Github user pauljackson commented on the issue: https://github.com/apache/tinkerpop/pull/547 Resubmitting this pull request.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user pauljackson opened a pull request:

          https://github.com/apache/tinkerpop/pull/547

          TINKERPOP-1589 Re-introduced CloseableIterator

          https://issues.apache.org/jira/browse/TINKERPOP-1589
          Add support for the closing of `Iterators` returned from
          `Vertex.vertices()` and `Vertex.edges()`.
          `Iterators` are closed once they are read to completion.
          Make `FlatMapStep` implement `AutoCloseable` in case iterator is not
          read to completion, should get closed when `Traversal` is closed.
          Remove unnecessary null check (null is never an instanceof).
          OLTP mode support only. More extensive changes required for OLAP.
          NOTE: Rethrowing checked Exception from `close()` as unchecked
          RuntimeException in order to retain `Step.reset()` and
          `AbstractStep.processNextStart()` signatures.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/pauljackson/tinkerpop tp32

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/547.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #547


          commit c8d5ff9db6ec8908abfad434c4fb339c5ad53468
          Author: PaulJackson123 <pauljackson123@verizon.net>
          Date: 2017-01-26T01:43:22Z

          TINKERPOP-1589 Re-introduced CloseableIterator

          https://issues.apache.org/jira/browse/TINKERPOP-1589
          Add support for the closing of Iterators returned from
          Vertex.vertices() and Vertex.edges().
          Iterators are closed once they are read to completion.
          Make FlatMapStep implement AutoCloseable in case iterator is not
          read to completion, should get closed when Traversal is closed.
          Remove unnecessary null check (null is never an instanceof).
          OLTP mode support only. More extensive changes required for OLAP.
          NOTE: Rethrowing checked Exception from close() as unchecked
          RuntimeException in order to retain Step.reset() and
          AbstractStep.processNextStart() signatured.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user pauljackson opened a pull request: https://github.com/apache/tinkerpop/pull/547 TINKERPOP-1589 Re-introduced CloseableIterator https://issues.apache.org/jira/browse/TINKERPOP-1589 Add support for the closing of `Iterators` returned from `Vertex.vertices()` and `Vertex.edges()`. `Iterators` are closed once they are read to completion. Make `FlatMapStep` implement `AutoCloseable` in case iterator is not read to completion, should get closed when `Traversal` is closed. Remove unnecessary null check (null is never an instanceof). OLTP mode support only. More extensive changes required for OLAP. NOTE: Rethrowing checked Exception from `close()` as unchecked RuntimeException in order to retain `Step.reset()` and `AbstractStep.processNextStart()` signatures. You can merge this pull request into a Git repository by running: $ git pull https://github.com/pauljackson/tinkerpop tp32 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/547.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #547 commit c8d5ff9db6ec8908abfad434c4fb339c5ad53468 Author: PaulJackson123 <pauljackson123@verizon.net> Date: 2017-01-26T01:43:22Z TINKERPOP-1589 Re-introduced CloseableIterator https://issues.apache.org/jira/browse/TINKERPOP-1589 Add support for the closing of Iterators returned from Vertex.vertices() and Vertex.edges(). Iterators are closed once they are read to completion. Make FlatMapStep implement AutoCloseable in case iterator is not read to completion, should get closed when Traversal is closed. Remove unnecessary null check (null is never an instanceof). OLTP mode support only. More extensive changes required for OLAP. NOTE: Rethrowing checked Exception from close() as unchecked RuntimeException in order to retain Step.reset() and AbstractStep.processNextStart() signatured.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/521

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/521
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twilmes commented on the issue:

          https://github.com/apache/tinkerpop/pull/521

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the issue: https://github.com/apache/tinkerpop/pull/521 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lvca commented on the issue:

          https://github.com/apache/tinkerpop/pull/521

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user lvca commented on the issue: https://github.com/apache/tinkerpop/pull/521 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/521

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/521 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/521#discussion_r94782450

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java —
          @@ -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 {
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/521#discussion_r94782450 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java — @@ -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 { — End diff – changes are made now - thanks - didn't know that stuff.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/521#discussion_r94776952

          — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java —
          @@ -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 {
          — End diff –

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

          ```
          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:

          ```
          for(final Traversal.Admin<?,?> traversal : this.getLocalChildren())

          { traversal.close() }

          for(final Traversal.Admin<?,? traversal: this.getGlobalChildren())

          { traversal.close(); }

          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/521#discussion_r94776952 — Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java — @@ -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 { — End diff – This is bad. `Steps` that are `TraversalParents` are responsible for propagating operations on them to those below. I would do the following: ``` 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: ``` for(final Traversal.Admin<?,?> traversal : this.getLocalChildren()) { traversal.close() } for(final Traversal.Admin<?,? traversal: this.getGlobalChildren()) { traversal.close(); } ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/521

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/521 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/521

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/521 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/521

          TINKERPOP-1589 Re-introduced CloseableIterator

          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`.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1589

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/521.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #521


          commit 3597dc5f06407a8b1ada0e49a52b36b2f31b8a34
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-01-04T21:14:23Z

          TINKERPOP-1589 Re-introduced CloseableIterator

          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.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/521 TINKERPOP-1589 Re-introduced CloseableIterator 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`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1589 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/521.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #521 commit 3597dc5f06407a8b1ada0e49a52b36b2f31b8a34 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-01-04T21:14:23Z TINKERPOP-1589 Re-introduced CloseableIterator 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.
          Hide
          spmallette stephen mallette added a comment -

          Those suggestions sound reasonable.

          I think I will go back on some of the stuff I said above in the description. I don't think we need to change the vertices() and edges() API for 3.3.0 anymore. The return types can probably just remain as Iterator. I don't think there is much user confusion to avoid here as we don't encourage users operating at the Graph Structure API level. Everything is done through the Traversal API. Therefore, they wouldn't be calling those methods on Graph in the first place.

          Show
          spmallette stephen mallette added a comment - Those suggestions sound reasonable. I think I will go back on some of the stuff I said above in the description. I don't think we need to change the vertices() and edges() API for 3.3.0 anymore. The return types can probably just remain as Iterator . I don't think there is much user confusion to avoid here as we don't encourage users operating at the Graph Structure API level. Everything is done through the Traversal API. Therefore, they wouldn't be calling those methods on Graph in the first place.
          Hide
          pauljackson Paul Jackson added a comment -

          This all sounds good.

          If you chose to make those methods return CloseableIterator, you could make it slightly easier for providers by giving CloseableIterator:
          1) A default close() that does nothing, and
          2) A static CloseableIterator<E> asCloseableIterator(Iterator<E> iterator) method that wraps the supplied Iterator with a default implementation (if the supplied iterator wasn't instanceof CloseableIterator).

          Show
          pauljackson Paul Jackson added a comment - This all sounds good. If you chose to make those methods return CloseableIterator, you could make it slightly easier for providers by giving CloseableIterator: 1) A default close() that does nothing, and 2) A static CloseableIterator<E> asCloseableIterator(Iterator<E> iterator) method that wraps the supplied Iterator with a default implementation (if the supplied iterator wasn't instanceof CloseableIterator).

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development