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

Configure Console for no timeout on remote requests

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.2-incubating
    • Fix Version/s: 3.1.3, 3.2.1
    • Component/s: console
    • Labels:
      None

      Description

      The console comes with a default timeout of 3 minutes for remote requests to Gremlin Server. You can change that value with:

      :remote config timeout 60000
      

      or you can make it "basically" indefinite with:

      :remote config timeout max
      

      I think "max" is kinda weird now that I look at it. That basically sets a time out for Integer.MAX_VALUE when you really just want to wait indefinitely and have no timeout at all. I guess "max" is just a sort of a bad word. It seems like it would be good to deprecate "max" in favor of:

      :remote config timeout none
      

      which is more in line with what someone actually wants to have when they are doing "max".

      Change the default timeout from 3 minutes to "none". It seems to be the more expected default. Can't think of a circumstance in the context of the console, where the client shouldn't just wait for the server's response.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/incubator-tinkerpop/pull/294

          TINKERPOP-1267 Added option for "none" on remote timeouts.

          Added some tests to validate the new timeout setting and did some manual tests:

          ```text
          gremlin> :remote connect tinkerpop.server conf/remote.yaml
          ==>Connected - localhost/127.0.0.1:8182
          gremlin> :remote config timeout max
          ==>Set remote timeout to 2147483647ms
          gremlin> :remote config timeout none
          ==>Remote timeout is disable
          gremlin> :remote config timeout 10000
          ==>Set remote timeout to 10000ms
          gremlin> :> Thread.sleep(9000)
          ==>null
          gremlin> :> Thread.sleep(11000)
          Request timed out while processing - increase the timeout with the :remote command
          Display stack trace? [yN] n
          ```

          Successful run of `mvn clean install && mvn verify -pl gremlin-console -DskipIntegrationTests=false`

          VOTE +1

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

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

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

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



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/294 TINKERPOP-1267 Added option for "none" on remote timeouts. Added some tests to validate the new timeout setting and did some manual tests: ```text gremlin> :remote connect tinkerpop.server conf/remote.yaml ==>Connected - localhost/127.0.0.1:8182 gremlin> :remote config timeout max ==>Set remote timeout to 2147483647ms gremlin> :remote config timeout none ==>Remote timeout is disable gremlin> :remote config timeout 10000 ==>Set remote timeout to 10000ms gremlin> :> Thread.sleep(9000) ==>null gremlin> :> Thread.sleep(11000) Request timed out while processing - increase the timeout with the :remote command Display stack trace? [yN] n ``` Successful run of `mvn clean install && mvn verify -pl gremlin-console -DskipIntegrationTests=false` VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-1267 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/294.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 #294
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/294#issuecomment-214851656

          Standard tests: passed
          Integration tests: passed
          User docs: successfully built

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/294#issuecomment-214851656 Standard tests: passed Integration tests: passed User docs: successfully built VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tinkerpop/pull/294#discussion_r61664727

          — Diff: gremlin-console/src/main/java/org/apache/tinkerpop/gremlin/console/groovy/plugin/DriverRemoteAcceptor.java —
          @@ -104,13 +113,14 @@ public Object configure(final List<String> args) throws RemoteException {
          final List<String> arguments = args.subList(1, args.size());

          if (option.equals(TOKEN_TIMEOUT)) {

          • final String errorMessage = "The timeout option expects a positive integer representing milliseconds or 'max' as an argument";
            + final String errorMessage = "The timeout option expects a positive integer representing milliseconds or 'none' as an argument";
            if (arguments.size() != 1) throw new RemoteException(errorMessage);
            try {
          • final int to = arguments.get(0).equals(TOKEN_MAX) ? Integer.MAX_VALUE : Integer.parseInt(arguments.get(0));
          • if (to <= 0) throw new RemoteException(errorMessage);
          • this.timeout = to;
          • return "Set remote timeout to " + to + "ms";
            + // first check for MAX timeout then NONE and finally parse the config to int. "max" is now "deprecated"
            + // in the sense that it will no longer be promoted. support for it will be removed at a later date
            + timeout = arguments.get(0).equals(TOKEN_MAX) ? Integer.MAX_VALUE :
            + arguments.get(0).equals(TOKEN_NONE) ? NO_TIMEOUT : Integer.parseInt(arguments.get(0));
            + return timeout == NO_TIMEOUT ? "Remote timeout is disable" : "Set remote timeout to " + timeout + "ms";
              • End diff –

          Missing the check here for `if (timeout < 0) throw new RemoteException(errorMessage);` so you can end up with
          ```
          gremlin> :remote config timeout -2
          ==>Set remote timeout to -2ms
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/294#discussion_r61664727 — Diff: gremlin-console/src/main/java/org/apache/tinkerpop/gremlin/console/groovy/plugin/DriverRemoteAcceptor.java — @@ -104,13 +113,14 @@ public Object configure(final List<String> args) throws RemoteException { final List<String> arguments = args.subList(1, args.size()); if (option.equals(TOKEN_TIMEOUT)) { final String errorMessage = "The timeout option expects a positive integer representing milliseconds or 'max' as an argument"; + final String errorMessage = "The timeout option expects a positive integer representing milliseconds or 'none' as an argument"; if (arguments.size() != 1) throw new RemoteException(errorMessage); try { final int to = arguments.get(0).equals(TOKEN_MAX) ? Integer.MAX_VALUE : Integer.parseInt(arguments.get(0)); if (to <= 0) throw new RemoteException(errorMessage); this.timeout = to; return "Set remote timeout to " + to + "ms"; + // first check for MAX timeout then NONE and finally parse the config to int. "max" is now "deprecated" + // in the sense that it will no longer be promoted. support for it will be removed at a later date + timeout = arguments.get(0).equals(TOKEN_MAX) ? Integer.MAX_VALUE : + arguments.get(0).equals(TOKEN_NONE) ? NO_TIMEOUT : Integer.parseInt(arguments.get(0)); + return timeout == NO_TIMEOUT ? "Remote timeout is disable" : "Set remote timeout to " + timeout + "ms"; End diff – Missing the check here for `if (timeout < 0) throw new RemoteException(errorMessage);` so you can end up with ``` gremlin> :remote config timeout -2 ==>Set remote timeout to -2ms ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/294#issuecomment-215934896

          Other than that nit, everything else checked out ok.
          The fix for that is trivial, so I'll go ahead and vote.
          I inspected the code, reviewed the docs, ran the tests.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/294#issuecomment-215934896 Other than that nit, everything else checked out ok. The fix for that is trivial, so I'll go ahead and vote. I inspected the code, reviewed the docs, ran the tests. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/incubator-tinkerpop/pull/294

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

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development