> On March 5, 2014, 4:39 p.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. > > Bill Havanki wrote: > 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.)
the default Shell constructor throws IOException. In this case though, it looks like the try block is just there to handle shutdown. If the constructor throws there's nothing to shutdown. - Sean ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18773/#review36234 ----------------------------------------------------------- On March 5, 2014, 4:29 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18773/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 4:29 p.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 > >
