Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-4128

ShellServerIT#scansWithClassLoaderContext fails consistently

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0
    • Component/s: test
    • Labels:
      None

      Activity

      Hide
      elserj Josh Elser added a comment -

      Dave Marion, do you have any time to look at this? Just ran into this, still failing 5 months later on master.

      Show
      elserj Josh Elser added a comment - Dave Marion , do you have any time to look at this? Just ran into this, still failing 5 months later on master.
      Hide
      dlmarion Dave Marion added a comment -

      I will try to look at it this week.

      Show
      dlmarion Dave Marion added a comment - I will try to look at it this week.
      Hide
      elserj Josh Elser added a comment -

      Thanks, Dave Marion. Here's a re-ping for the coming week

      Show
      elserj Josh Elser added a comment - Thanks, Dave Marion . Here's a re-ping for the coming week
      Hide
      dlmarion Dave Marion added a comment -

      Which branch? Master, or is this failing in 1.8 also?

      Show
      dlmarion Dave Marion added a comment - Which branch? Master, or is this failing in 1.8 also?
      Hide
      elserj Josh Elser added a comment -

      Just master (meant to tell that by the 2.0.0 fixVersion, but I can see how that would be confusing)

      Show
      elserj Josh Elser added a comment - Just master (meant to tell that by the 2.0.0 fixVersion, but I can see how that would be confusing)
      Hide
      dlmarion Dave Marion added a comment - - edited

      I think I know what's going on, but I don't know where to look to fix it. I put in a bunch of debug and found that the classloaders that are loading the iterators for the test are the same (same instance). Furthermore, they are not VFS classloader or URL classloaders, they are of type sun.misc.Launcher$AppClassLoader, which (according to [1]) is the class loader for the application (in this case the TabletServer). Is there a way to print out the classpath of the java process that is started for MAC?

      [1] https://blogs.oracle.com/sundararajan/entry/understanding_java_class_loading

      Show
      dlmarion Dave Marion added a comment - - edited I think I know what's going on, but I don't know where to look to fix it. I put in a bunch of debug and found that the classloaders that are loading the iterators for the test are the same (same instance). Furthermore, they are not VFS classloader or URL classloaders, they are of type sun.misc.Launcher$AppClassLoader, which (according to [1] ) is the class loader for the application (in this case the TabletServer). Is there a way to print out the classpath of the java process that is started for MAC? [1] https://blogs.oracle.com/sundararajan/entry/understanding_java_class_loading
      Hide
      elserj Josh Elser added a comment -

      Dave Marion, assuming you have the ability to tweak the code when re-running, you can try to print out argList

      https://github.com/apache/accumulo/blob/fd67a6c85a93bb26817700982954232b65a01abf/minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java#L291-L309

      I don't think we have anything to conveniently do this via MiniAccumuloConfigImpl (a debug mode would be boss).

      Show
      elserj Josh Elser added a comment - Dave Marion , assuming you have the ability to tweak the code when re-running, you can try to print out argList https://github.com/apache/accumulo/blob/fd67a6c85a93bb26817700982954232b65a01abf/minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java#L291-L309 I don't think we have anything to conveniently do this via MiniAccumuloConfigImpl (a debug mode would be boss).
      Hide
      dlmarion Dave Marion added a comment -

      Thanks, I'll give it a shot in the morning.

      Show
      dlmarion Dave Marion added a comment - Thanks, I'll give it a shot in the morning.
      Hide
      dlmarion Dave Marion added a comment -

      cc: Christopher Tubbs

      Ok, I think I found the problem. The shell command `setscaniter` appears to be non-existent in the master branch. The test was not checking the output of the shell to confirm that the iterator was applied. More concerning is that SetScanIterCommand is not marked deprecated in the 1.8 branch. The new command looks to be `setiter -scan`.

      Show
      dlmarion Dave Marion added a comment - cc: Christopher Tubbs Ok, I think I found the problem. The shell command `setscaniter` appears to be non-existent in the master branch. The test was not checking the output of the shell to confirm that the iterator was applied. More concerning is that SetScanIterCommand is not marked deprecated in the 1.8 branch. The new command looks to be `setiter -scan`.
      Hide
      elserj Josh Elser added a comment -

      The shell command `setscaniter` appears to be non-existent in the master branch. The test was not checking the output of the shell to confirm that the iterator was applied.

      Wow, good find, Dave! I totally missed that scrolling through the output.

      More concerning is that SetScanIterCommand is not marked deprecated in the 1.8 branch. The new command looks to be `setiter -scan`.

      By the letter of the law, shell commands are presently covered by semver so they can technically be deleted immediately (playing devil's advocate – I think this is terrible and shell commands should also be covered in our public api). Did you happen to do any digging to figure out what commit/issue changed this in master?

      Show
      elserj Josh Elser added a comment - The shell command `setscaniter` appears to be non-existent in the master branch. The test was not checking the output of the shell to confirm that the iterator was applied. Wow, good find, Dave! I totally missed that scrolling through the output. More concerning is that SetScanIterCommand is not marked deprecated in the 1.8 branch. The new command looks to be `setiter -scan`. By the letter of the law, shell commands are presently covered by semver so they can technically be deleted immediately (playing devil's advocate – I think this is terrible and shell commands should also be covered in our public api). Did you happen to do any digging to figure out what commit/issue changed this in master?
      Hide
      dlmarion Dave Marion added a comment -

      By the letter of the law, shell commands are presently covered by semver

      They are? or are they not? If they are covered, then they have to be deprecated in a prior release and then removed in a major release. If not, then anything is fair game.

      Did you happen to do any digging to figure out what commit/issue changed this in master?

      I did not.

      Show
      dlmarion Dave Marion added a comment - By the letter of the law, shell commands are presently covered by semver They are? or are they not? If they are covered, then they have to be deprecated in a prior release and then removed in a major release. If not, then anything is fair game. Did you happen to do any digging to figure out what commit/issue changed this in master? I did not.
      Hide
      elserj Josh Elser added a comment -

      By the letter of the law, shell commands are presently covered by semver

      They are? or are they not? If they are covered, then they have to be deprecated in a prior release and then removed in a major release. If not, then anything is fair game.

      Oh my, bad missing word. They are not covered

      Billie did a quick search and mentioned that it was removed in ACCUMULO-2589 and that it was deprecated. Even so, feels bad to me.

      Show
      elserj Josh Elser added a comment - By the letter of the law, shell commands are presently covered by semver They are? or are they not? If they are covered, then they have to be deprecated in a prior release and then removed in a major release. If not, then anything is fair game. Oh my, bad missing word. They are not covered Billie did a quick search and mentioned that it was removed in ACCUMULO-2589 and that it was deprecated. Even so, feels bad to me.
      Hide
      ctubbsii Christopher Tubbs added a comment -

      SetIterCommand was deprecated. It was replaced with SetShellIterCommand. This is revealed in the warning message when the command is executed:

      Shell.log.warn("Deprecated, use " + new SetShellIterCommand().getName());

      Show
      ctubbsii Christopher Tubbs added a comment - SetIterCommand was deprecated. It was replaced with SetShellIterCommand. This is revealed in the warning message when the command is executed: Shell.log.warn("Deprecated, use " + new SetShellIterCommand().getName());
      Hide
      dlmarion Dave Marion added a comment -

      I'll change the test to use setshelliter. Thanks Christopher Tubbs.

      Show
      dlmarion Dave Marion added a comment - I'll change the test to use setshelliter. Thanks Christopher Tubbs .
      Hide
      dlmarion Dave Marion added a comment -

      Test changed and committed.

      Show
      dlmarion Dave Marion added a comment - Test changed and committed.

        People

        • Assignee:
          dlmarion Dave Marion
          Reporter:
          ecn Eric Newton
        • Votes:
          0 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Time Tracking

            Estimated:
            Original Estimate - Not Specified
            Not Specified
            Remaining:
            Remaining Estimate - 0h
            0h
            Logged:
            Time Spent - 1.5h
            1.5h

              Development