> On March 5, 2014, 11:39 a.m., Mike Drob wrote: > > core/src/test/java/org/apache/accumulo/core/util/shell/ShellTest.java, line > > 114 > > <https://reviews.apache.org/r/18773/diff/1/?file=510871#file510871line114> > > > > Do we need a null check?
Eh, it couldn't hurt, but the shell should have been set in the @Before method. Without the check, the test could fail with an NPE, which would mean that something was very wrong with the test. So, overall, I don't know if a check would help. > On March 5, 2014, 11:39 a.m., Mike Drob wrote: > > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java, line 215 > > <https://reviews.apache.org/r/18773/diff/1/?file=510872#file510872line215> > > > > Do we need a null check? Same response as above. > On March 5, 2014, 11:39 a.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java, line 460 > > <https://reviews.apache.org/r/18773/diff/1/?file=510868#file510868line460> > > > > Should move the assignment inside of the try-block. Why? Even now it's the default no-arg constructor, it can't even throw anything. (This is my usual idiom, so if there is a standard reason why it's best to do the assignment in the try that I don't know, I definitely want to learn it.) > On March 5, 2014, 11:39 a.m., Mike Drob wrote: > > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java, lines > > 188-192 > > <https://reviews.apache.org/r/18773/diff/1/?file=510872#file510872line188> > > > > Why did the tracer get removed from here? Ah, I forgot I had made this change, sorry about that. The static @BeforeClass method already starts a tracer, and the static @AfterClass method terminates it. The traceProcess is a static field, so it applies across the whole test class, so spawning the tracer once for all the tests makes sense and is consistent. With the deleted lines here, each test spawns another tracer, replacing the last one's reference which never gets terminated. You end up with 40 or more tracer processes. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18773/#review36234 ----------------------------------------------------------- On March 5, 2014, 11:29 a.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18773/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 11:29 a.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2429 > https://issues.apache.org/jira/browse/ACCUMULO-2429 > > > Repository: accumulo > > > Description > ------- > > The JLine 2 ConsoleReader used by Shell spawns a thread which should be > cleaned up when done with the Shell. Otherwise, the thread leaks, taking up > resources when the shell is used programmatically. This commit adds a > shutdown() method to Shell for cleaning up the thread. This enables > ShellServerIT to pass reliably and not flood the OS with leaked threads. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java > 850816c7e1673d89ae8fd29818cf3322c3cd8b3b > core/src/test/java/org/apache/accumulo/core/util/shell/ShellConfigTest.java > df4d817aad50e3c34e6de02f68b4345af00bfac6 > > core/src/test/java/org/apache/accumulo/core/util/shell/ShellSetInstanceTest.java > 5a6cc8a0052b03cfa510b80b072408139428ccff > core/src/test/java/org/apache/accumulo/core/util/shell/ShellTest.java > bf203f77d4e6809bacdb82217e3c8d8aaeecbe48 > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java > da094e85ed3ebaeef2ac2cac5beb7b52299a49dc > > Diff: https://reviews.apache.org/r/18773/diff/ > > > Testing > ------- > > Ran shell-related unit tests successfully. Ran ShellServerIT successfully > under both Mac OS X and CentOS 6. Ran shell against live instance to ensure > it starts and stops normally. > > > Thanks, > > Bill Havanki > >
