Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1442

Killing session should make better attempt to cleanup

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.1.4
    • 3.1.5, 3.2.3
    • server
    • None

    Description

      When a session is killed it tries to rollback transactions prior to ending the session. If there is a long-run script (or perhaps a queue of jobs) then this close job just gets added to the queue. If that rollback job never executes then the transaction gets orphaned.

      Graph implementations tend to have their own methods for cleaning up these lingering transactions, but it would be better if the shutdown could occur in an orderly fashion.

      Attachments

        Activity

          Tried several different approaches to solving this - each were valiant attempts - but ultimately there's no perfect way to do this and we end up with the Graph implementation stuck with the having to clean up it's own open transactions. The problem is that a session must maintain a single thread associated with it. If you interrupt that thread in doing its work then you orphan the transaction because you can't then issue more work in that thread to rollback incomplete transactions. Perhaps this is another reason to decouple thread interruption from traversal interruption, however, thread interruption still seems like a necessary approach to killing the session because a script may have nothing to do with "traversal interruption". You may just be trying to break out of a Thread.sleep().

          Anyway, I've at least got this setup so that you can issue a "close" on a session and if there's a job blocking it will attempt to kill it - that's more than what we had before. In this way, that session thread won't run into oblivion on close waiting for a timeout to trigger. The timeout could be quite high in some cases (like for an OLAP traversal for example) and if enough of those hung around it wouldn't be healthy for the server.

          I was able to make the change without breaks and no new additions to the protocol. Running tests and then will issue PRs.

          spmallette Stephen Mallette added a comment - Tried several different approaches to solving this - each were valiant attempts - but ultimately there's no perfect way to do this and we end up with the Graph implementation stuck with the having to clean up it's own open transactions. The problem is that a session must maintain a single thread associated with it. If you interrupt that thread in doing its work then you orphan the transaction because you can't then issue more work in that thread to rollback incomplete transactions. Perhaps this is another reason to decouple thread interruption from traversal interruption, however, thread interruption still seems like a necessary approach to killing the session because a script may have nothing to do with "traversal interruption". You may just be trying to break out of a Thread.sleep() . Anyway, I've at least got this setup so that you can issue a "close" on a session and if there's a job blocking it will attempt to kill it - that's more than what we had before. In this way, that session thread won't run into oblivion on close waiting for a timeout to trigger. The timeout could be quite high in some cases (like for an OLAP traversal for example) and if enough of those hung around it wouldn't be healthy for the server. I was able to make the change without breaks and no new additions to the protocol. Running tests and then will issue PRs.
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

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

          TINKERPOP-1442 Improved session cleanup on client close [tp31]

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

          While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather than consuming the thread indefinitely (or until timeout whenever that is).

          All unit tests are good including gremlin-server integration tests.

          VOTE +1

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

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

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

          https://github.com/apache/tinkerpop/pull/412.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 #412


          commit e7d63381dcc0c53999f4270f0e6b9d4a56093673
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-13T22:10:09Z

          Improved session cleanup on client close.

          While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather thant consuming the thread indefinitely. TINKERPOP-1442


          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/412 TINKERPOP-1442 Improved session cleanup on client close [tp31] https://issues.apache.org/jira/browse/TINKERPOP-1442 While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather than consuming the thread indefinitely (or until timeout whenever that is). All unit tests are good including gremlin-server integration tests. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1442 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/412.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 #412 commit e7d63381dcc0c53999f4270f0e6b9d4a56093673 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-13T22:10:09Z Improved session cleanup on client close. While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather thant consuming the thread indefinitely. TINKERPOP-1442
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

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

          TINKERPOP-1442 Improved session cleanup on client close [master]

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

          see #412 for more description

          All unit tests and gremlin-server integration tests are passing on this branch.

          VOTE +1

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

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

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

          https://github.com/apache/tinkerpop/pull/413.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 #413


          commit e7d63381dcc0c53999f4270f0e6b9d4a56093673
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-13T22:10:09Z

          Improved session cleanup on client close.

          While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather thant consuming the thread indefinitely. TINKERPOP-1442

          commit cd07845479c5e945683cb5e49b92a74277295e55
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-13T22:17:23Z

          Merge branch 'TINKERPOP-1442' into TINKERPOP-1442-master


          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/413 TINKERPOP-1442 Improved session cleanup on client close [master] https://issues.apache.org/jira/browse/TINKERPOP-1442 see #412 for more description All unit tests and gremlin-server integration tests are passing on this branch. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1442 -master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/413.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 #413 commit e7d63381dcc0c53999f4270f0e6b9d4a56093673 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-13T22:10:09Z Improved session cleanup on client close. While not a perfect implementation, a long run job blocking a close request from the client will now at least get an attempt at interruption rather thant consuming the thread indefinitely. TINKERPOP-1442 commit cd07845479c5e945683cb5e49b92a74277295e55 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-13T22:17:23Z Merge branch ' TINKERPOP-1442 ' into TINKERPOP-1442 -master
          githubbot ASF GitHub Bot added a comment -

          Github user PommeVerte commented on the issue:

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

          VOTE +1 this is a nice fix to have. Unit tests passed for me too.

          githubbot ASF GitHub Bot added a comment - Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/413 VOTE +1 this is a nice fix to have. Unit tests passed for me too.
          githubbot ASF GitHub Bot added a comment -

          Github user PommeVerte commented on the issue:

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

          VOTE: +1

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

          Github user davebshow commented on the issue:

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

          Stop accepting transactions after a close makes sense to me. This branch builds successfully using mvn clean install. VOTE +1

          githubbot ASF GitHub Bot added a comment - Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/413 Stop accepting transactions after a close makes sense to me. This branch builds successfully using mvn clean install. VOTE +1
          githubbot ASF GitHub Bot added a comment -

          Github user davebshow commented on the issue:

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

          This branch builds successfully using mvn clean install. VOTE +1

          githubbot ASF GitHub Bot added a comment - Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/412 This branch builds successfully using mvn clean install. VOTE +1
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

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

          I'm just going to throw my $0.02 out there. Maybe I don't understand the use case, but this seems strange and unexpected to me. I don't understand why or how a completely separate client would affect another. Isn't one of the reasons for using sessions is that a client can come in and out while retaining state? How would a client safely disconnect and reconnect later without killing the session and other clients?

          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/413 I'm just going to throw my $0.02 out there. Maybe I don't understand the use case, but this seems strange and unexpected to me. I don't understand why or how a completely separate client would affect another. Isn't one of the reasons for using sessions is that a client can come in and out while retaining state? How would a client safely disconnect and reconnect later without killing the session and other clients?
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user asfgit closed the pull request at:

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

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

          People

            spmallette Stephen Mallette
            spmallette Stephen Mallette
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: