You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Bill Havanki <bh...@clouderagovt.com> on 2014/03/05 17:29:20 UTC
Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread cleanup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18773/
-----------------------------------------------------------
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
Re: Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread
cleanup
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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
>
>
Re: Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread
cleanup
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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.
>
> 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.)
>
> Sean Busbey wrote:
> 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.
Don't know how I missed that constructor. Please disregard my last comment.
It is possible for that thread to start before the JLine ConsoleReader constructor throws IOException [1]. If it does, we've got no reference to the instance to tell it to stop the thread, so we're screwed. Ergo, there is no point in having the constructor occur in the try-catch.
[1] https://github.com/jline/jline2/blob/master/src/main/java/jline/console/ConsoleReader.java
- 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
>
>
Re: Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread
cleanup
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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
>
>
Re: Review Request 18773: ACCUMULO-2429 - Add shell shutdown for thread
cleanup
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18773/#review36234
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java
<https://reviews.apache.org/r/18773/#comment67175>
Should move the assignment inside of the try-block.
core/src/test/java/org/apache/accumulo/core/util/shell/ShellTest.java
<https://reviews.apache.org/r/18773/#comment67176>
Do we need a null check?
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java
<https://reviews.apache.org/r/18773/#comment67174>
Why did the tracer get removed from here?
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java
<https://reviews.apache.org/r/18773/#comment67177>
Do we need a null check?
- Mike Drob
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
>
>