accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread cleanup
Date Wed, 05 Mar 2014 16:50:10 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message