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

GremlinGroovyScriptEngineOverGraphTest failures

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-incubating
    • Fix Version/s: 3.1.1-incubating
    • Component/s: test-suite
    • Labels:
      None

      Description

      I am running these tests for the first time and are encountering some issues.

      The first is pretty trivial, many places in test the vertex id is not wrapped in single quotes resulting in,

      javax.script.ScriptException: org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
      Script4.groovy: 1: illegal colon after argument expression;
         solution: a complex label expression before a colon must be parenthesized @ line 1, column 26.
         isVadas(g.V(PUBLIC.person:::1).next())
      

      The second issue is in the multi threaded tests. The implicit transactions are never commit nor rolled backed. The tests create 500 threads/transactions. My connection pool default to 100 connections and are thus starved hanging the test as the resources are not released.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160720475

        merged by CTR as this is pretty simple stuff - just adds a couple transaction management lines to the tests. thanks.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160720475 merged by CTR as this is pretty simple stuff - just adds a couple transaction management lines to the tests. thanks.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

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

        Github user pietermartin commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160251042

        Not sure if I followed the correct procedure for updating a pull request as no notification was sent. However the amendment commit is on now on this pull request.

        Show
        githubbot ASF GitHub Bot added a comment - Github user pietermartin commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160251042 Not sure if I followed the correct procedure for updating a pull request as no notification was sent. However the amendment commit is on now on this pull request.
        Hide
        pietermartin pieter martin added a comment -

        Yeah, that was Intellij being intelligent. I have it on auto correct import statements.
        I'll change my Intellij settings and resubmit.

        Show
        pietermartin pieter martin added a comment - Yeah, that was Intellij being intelligent. I have it on auto correct import statements. I'll change my Intellij settings and resubmit.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160199453

        I'll take the lead to merge this by CTR once that little issue of style is resolved. Thanks for this.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/159#issuecomment-160199453 I'll take the lead to merge this by CTR once that little issue of style is resolved. Thanks for this.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/incubator-tinkerpop/pull/159#discussion_r46068829

        — Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineOverGraphTest.java —
        @@ -36,21 +36,10 @@
        import javax.script.CompiledScript;
        import javax.script.ScriptEngine;
        import javax.script.ScriptException;
        -import javax.script.SimpleBindings;
        -import java.util.ArrayList;
        -import java.util.Arrays;
        -import java.util.HashSet;
        -import java.util.List;
        -import java.util.Random;
        — End diff –

        as a matter of style, can you please amend your PR to not use wildcards?

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/159#discussion_r46068829 — Diff: gremlin-groovy-test/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineOverGraphTest.java — @@ -36,21 +36,10 @@ import javax.script.CompiledScript; import javax.script.ScriptEngine; import javax.script.ScriptException; -import javax.script.SimpleBindings; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.HashSet; -import java.util.List; -import java.util.Random; — End diff – as a matter of style, can you please amend your PR to not use wildcards?
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user pietermartin opened a pull request:

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

        TINKERPOP3-1000 Rollback the implicit transaction on multi threaded tests.

        Tested on TinkerGraph, which does not support transactions.
        Tested on Sqlg, which does support transactions, passes the test.

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

        $ git pull https://github.com/pietermartin/incubator-tinkerpop master

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

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


        commit 8bcebbaef66af670d9226be807a45943120d87c2
        Author: pieter <pieter.martin@gmail.com>
        Date: 2015-11-27T19:42:51Z

        Rollback the implicit transaction on multi threaded tests.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user pietermartin opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/159 TINKERPOP3-1000 Rollback the implicit transaction on multi threaded tests. Tested on TinkerGraph, which does not support transactions. Tested on Sqlg, which does support transactions, passes the test. You can merge this pull request into a Git repository by running: $ git pull https://github.com/pietermartin/incubator-tinkerpop master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/159.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 #159 commit 8bcebbaef66af670d9226be807a45943120d87c2 Author: pieter <pieter.martin@gmail.com> Date: 2015-11-27T19:42:51Z Rollback the implicit transaction on multi threaded tests.
        Hide
        pietermartin pieter martin added a comment -

        Sure...

        Show
        pietermartin pieter martin added a comment - Sure...
        Hide
        spmallette stephen mallette added a comment -

        Can we expect a PR for that fix?

        Show
        spmallette stephen mallette added a comment - Can we expect a PR for that fix?
        Hide
        pietermartin pieter martin added a comment -

        Ok, checked out master.

        They fixed the quotes issue but not the failure to commit/rollback the implicit transaction in the multi threaded tests.

        The following tests are hanging.

        GremlinGroovyScriptEngineOverGraphTest.shouldBeThreadSafe
        GremlinGroovyScriptEngineOverGraphTest.shouldBeThreadSafeOnCompiledScript

        If I add in a finally, then all is well for sqlg and TinkerGraph

        tr {
        ...
        finally {
            if (graph.features().graph().supportsTransactions())
                g.tx().rollback(); 
        }
        
        Show
        pietermartin pieter martin added a comment - Ok, checked out master. They fixed the quotes issue but not the failure to commit/rollback the implicit transaction in the multi threaded tests. The following tests are hanging. GremlinGroovyScriptEngineOverGraphTest.shouldBeThreadSafe GremlinGroovyScriptEngineOverGraphTest.shouldBeThreadSafeOnCompiledScript If I add in a finally, then all is well for sqlg and TinkerGraph tr { ... finally { if (graph.features().graph().supportsTransactions()) g.tx().rollback(); }
        Hide
        okram Marko A. Rodriguez added a comment -

        This was fixed by OrientDB guys. Please check with master/ and if it works, please close this ticket.

        Show
        okram Marko A. Rodriguez added a comment - This was fixed by OrientDB guys. Please check with master/ and if it works, please close this ticket.

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            pietermartin pieter martin
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development