Details

    Description

      If :install tanks:

      gremlin> :install org.apache.tinkerpop hadoop-gremlin 3.1.0-SNAPSHOT
      ==>Error grabbing Grapes -- [download failed: io.netty#netty;3.6.2.Final!netty.jar(bundle)]
      

      try to clean up the directory that gets created or else future attempts do this:

      gremlin> :install org.apache.tinkerpop hadoop-gremlin 3.1.0-SNAPSHOT
      ==>a module with the name hadoop-gremlin is already installed
      

      This is also a problem for bin/gremlin-server.sh -i

      Attachments

        Activity

          pluradj said he was going to take a look at this one. Here's some pointers to ease your work. For the console, I believe some additional logic here to "clean up" should solve the problem:

          https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy#L56

          for the server, similar logic would need to be injected here:

          https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java#L40

          I think it's that simple - hopefully it's not a bigger deal than that. Not sure that you can easily write tests for this stuff, but if you can come up with something that would be nice.

          spmallette Stephen Mallette added a comment - pluradj said he was going to take a look at this one. Here's some pointers to ease your work. For the console, I believe some additional logic here to "clean up" should solve the problem: https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy#L56 for the server, similar logic would need to be injected here: https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java#L40 I think it's that simple - hopefully it's not a bigger deal than that. Not sure that you can easily write tests for this stuff, but if you can come up with something that would be nice.
          pluradj Jason Plurad added a comment -

          I ended up testing this manually, using Titan as the example.

          Prep

          rm ~/.groovy/grapeConfig.xml
          rm -rf ~/.groovy/grapes/org.apache.giraph/
          rm -rf ~/.m2/repository/org/apache/giraph/
          

          Gremlin Server

          1. cd gremlin-server/target/apache-gremlin-server-3.0.2-SNAPSHOT-standalone/
          2. Attempt to install titan-all should fail
                ./bin/gremlin-server.sh -i com.thinkaurelius.titan titan-all 1.0.0
            
          3. Verify that plugin directory titan-all doesn't exist
                ls ./ext/titan-all
            
          4. Attempt to install existing plugin tinkergraph-gremlin should fail
                ./bin/gremlin-server.sh -i org.apache.tinkerpop tinkergraph-gremlin 3.0.2-SNAPSHOT
            
          5. Verify that plugin directory should still exist
                ls ./ext/tinkergraph-gremlin
            

          Gremlin Console

          1. cd gremlin-console/target/apache-gremlin-console-3.0.2-SNAPSHOT-standalone/
          2. Attempt to install titan-all should fail
                ./bin/gremlin.sh
                :install com.thinkaurelius.titan titan-all 1.0.0
                :q
            
          3. Verify that plugin directory titan-all doesn't exist
                ls ./ext/titan-all
            
          4. Attempt to install existing plugin tinkergraph-gremlin should fail
                ./bin/gremlin.sh
                :install org.apache.tinkerpop tinkergraph-gremlin 3.0.2-SNAPSHOT
                :q
            
          5. Verify that plugin directory should still exist
                ls ./ext/tinkergraph-gremlin
            
          pluradj Jason Plurad added a comment - I ended up testing this manually, using Titan as the example. Prep rm ~/.groovy/grapeConfig.xml rm -rf ~/.groovy/grapes/org.apache.giraph/ rm -rf ~/.m2/repository/org/apache/giraph/ Gremlin Server cd gremlin-server/target/apache-gremlin-server-3.0.2-SNAPSHOT-standalone/ Attempt to install titan-all should fail ./bin/gremlin-server.sh -i com.thinkaurelius.titan titan-all 1.0.0 Verify that plugin directory titan-all doesn't exist ls ./ext/titan-all Attempt to install existing plugin tinkergraph-gremlin should fail ./bin/gremlin-server.sh -i org.apache.tinkerpop tinkergraph-gremlin 3.0.2-SNAPSHOT Verify that plugin directory should still exist ls ./ext/tinkergraph-gremlin Gremlin Console cd gremlin-console/target/apache-gremlin-console-3.0.2-SNAPSHOT-standalone/ Attempt to install titan-all should fail ./bin/gremlin.sh :install com.thinkaurelius.titan titan-all 1.0.0 :q Verify that plugin directory titan-all doesn't exist ls ./ext/titan-all Attempt to install existing plugin tinkergraph-gremlin should fail ./bin/gremlin.sh :install org.apache.tinkerpop tinkergraph-gremlin 3.0.2-SNAPSHOT :q Verify that plugin directory should still exist ls ./ext/tinkergraph-gremlin
          githubbot ASF GitHub Bot added a comment -

          GitHub user pluradj opened a pull request:

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

          TINKERPOP3-858 Cleanup after failed plugin install

          https://issues.apache.org/jira/browse/TINKERPOP3-858 see manual testing steps in the JIRA

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

          $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-858

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

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


          commit 955a98f7af541cc10674defeca53c4870ca8f441
          Author: Jason Plurad <pluradj@apache.org>
          Date: 2015-10-16T14:27:51Z

          TINKERPOP3-858 Cleanup after failed plugin install


          githubbot ASF GitHub Bot added a comment - GitHub user pluradj opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/110 TINKERPOP3-858 Cleanup after failed plugin install https://issues.apache.org/jira/browse/TINKERPOP3-858 see manual testing steps in the JIRA You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP3-858 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/110.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 #110 commit 955a98f7af541cc10674defeca53c4870ca8f441 Author: Jason Plurad <pluradj@apache.org> Date: 2015-10-16T14:27:51Z TINKERPOP3-858 Cleanup after failed plugin install
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148755126

          VOTE: +1

          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148755126 VOTE: +1
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tinkerpop/pull/110#discussion_r42271237

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy —
          @@ -53,6 +53,12 @@ class InstallCommand extends CommandSupport

          { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") }

          catch (Exception ex) {
          + if (!(ex instanceof IllegalStateException)) {
          + // IllegalStateException is thrown if a module with the same name is already installed.
          + final def uninstall = new UninstallCommand(shell, mediator)
          + final List<String> module = Collections.singletonList(artifact.getArtifact())
          + uninstall.execute(module)
          — End diff –

          Can uninstall throw an exception?

          githubbot ASF GitHub Bot added a comment - Github user mhfrantz commented on a diff in the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#discussion_r42271237 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy — @@ -53,6 +53,12 @@ class InstallCommand extends CommandSupport { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") } catch (Exception ex) { + if (!(ex instanceof IllegalStateException)) { + // IllegalStateException is thrown if a module with the same name is already installed. + final def uninstall = new UninstallCommand(shell, mediator) + final List<String> module = Collections.singletonList(artifact.getArtifact()) + uninstall.execute(module) — End diff – Can uninstall throw an exception?
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148801984

          @dkuppitz `DependencyGrabber.copyDependenciesToPath()` throws two exceptions explicitly, `IOException` and `IllegalStateException`. `RuntimeException` occurs if there is an error grabbing grapes. This is the initial bug report, and its failed directory should be rolled back.

          `IOException` occurs if there are problems creating the directories under `ext`. Rollback should occur here, but most likely there isn't even a directory to delete.

          `IllegalStateException` occurs if plugin module already exists. For example, `tinkergraph-gremlin` is installed by default out of the box. If the code doesn't catch the `IllegalStateException`, `tinkergraph-gremlin` would get uninstalled if the user tried to install it a second time.

          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148801984 @dkuppitz `DependencyGrabber.copyDependenciesToPath()` throws two exceptions explicitly, `IOException` and `IllegalStateException`. `RuntimeException` occurs if there is an error grabbing grapes. This is the initial bug report, and its failed directory should be rolled back. `IOException` occurs if there are problems creating the directories under `ext`. Rollback should occur here, but most likely there isn't even a directory to delete. `IllegalStateException` occurs if plugin module already exists. For example, `tinkergraph-gremlin` is installed by default out of the box. If the code doesn't catch the `IllegalStateException`, `tinkergraph-gremlin` would get uninstalled if the user tried to install it a second time.
          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/110#discussion_r42276002

          — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy —
          @@ -53,6 +53,12 @@ class InstallCommand extends CommandSupport

          { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") }

          catch (Exception ex) {
          + if (!(ex instanceof IllegalStateException)) {
          + // IllegalStateException is thrown if a module with the same name is already installed.
          + final def uninstall = new UninstallCommand(shell, mediator)
          + final List<String> module = Collections.singletonList(artifact.getArtifact())
          + uninstall.execute(module)
          — End diff –

          No, I didn't see any exceptions declared or thrown in the class.

          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/110#discussion_r42276002 — Diff: gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy — @@ -53,6 +53,12 @@ class InstallCommand extends CommandSupport { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") } catch (Exception ex) { + if (!(ex instanceof IllegalStateException)) { + // IllegalStateException is thrown if a module with the same name is already installed. + final def uninstall = new UninstallCommand(shell, mediator) + final List<String> module = Collections.singletonList(artifact.getArtifact()) + uninstall.execute(module) — End diff – No, I didn't see any exceptions declared or thrown in the class.
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148830986

          +1

          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148830986 +1
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148851150

          If @pluradj votes +1, then there will be three. Thus, @pluradj – if you think your work is solid, then +1 it and tally the vote and merge. Good luck.

          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148851150 If @pluradj votes +1, then there will be three. Thus, @pluradj – if you think your work is solid, then +1 it and tally the vote and merge. Good luck.
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148955829

          Thanks for the comments. I've centralized the rollback logic into DependencyGrabber, and added a couple unit tests.

          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148955829 Thanks for the comments. I've centralized the rollback logic into DependencyGrabber, and added a couple unit tests.
          githubbot ASF GitHub Bot added a comment -

          Github user pluradj commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148955862

          Vote: +1

          githubbot ASF GitHub Bot added a comment - Github user pluradj commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148955862 Vote: +1
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148958687

          You have 3 +1 votes. Please merge and if this is going into tp30, don't forget to merge to master as well. Good luck.

          githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/110#issuecomment-148958687 You have 3 +1 votes. Please merge and if this is going into tp30, don't forget to merge to master as well. Good luck.
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          People

            pluradj Jason Plurad
            spmallette Stephen Mallette
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: