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 19:40:48 GMT


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


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