accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 30382: ACCUMULO-3514 Use auto service for start
Date Sat, 31 Jan 2015 07:21:29 GMT


> On Jan. 30, 2015, 5:40 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/util/Classpath.java, line 33
> > <https://reviews.apache.org/r/30382/diff/4/?file=842636#file842636line33>
> >
> >     I'm kind of not a fan of this cyclic dependency, but I'm not sure there is a
better solution.

I'm not either, but technically, we already had a dependency cycle here... which is why we
were loading the classes via reflection. I did a first attempt at putting the annotated classes
in the same jar as the Main method itself, but that did not work, due to a chicken-and-egg
problem with the interface being defined in the same jar as we were trying to run the annotation
processor on. Essentially, it resulted in compile-time CNFEs, so I had to move it to core.
It was either this, or have one keyword function treated specially, which seemed in opposition
to your previous suggestion (and my preference) to make annotated classes for those extra
keywords also.


> On Jan. 30, 2015, 5:40 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/util/Jar.java, line 39
> > <https://reviews.apache.org/r/30382/diff/4/?file=842639#file842639line39>
> >
> >     Should classes be exiting directly? Maybe it would be better to return a boolean
or status of some sort. Can be done in a follow on issue.

Primarily, I did this to preserve existing behavior. However, due to the nature of the `execute`
function on this interface, it really is supposed to function like a `main()` method, so unless
we want to define additional semantics, this is the easiest way to send out an exit code that
is non-zero. The only purpose of this interface is to automate management of main classes
associated with particular keywords, we were maintaining manually, so nobody should be calling
execute themselves (or, if they do, they should expect it to behave similarly to `main()`,
which frequently call `System.exit(n)`.

What would be *really* great is if Java itself allowed `public static int main`, like how
some versions of C allow `int main` instead of `void main`. Then we could do more fancy things
without inventing our own semantics.


> On Jan. 30, 2015, 5:40 p.m., Mike Drob wrote:
> > start/src/main/java/org/apache/accumulo/start/Main.java, line 270
> > <https://reviews.apache.org/r/30382/diff/4/?file=842665#file842665line270>
> >
> >     This could declare the return a SortedMap? Or not actually bother returning
a sorted map.

It doesn't matter. We don't care if it is sorted or not, and the set is so small, that pretty
much any implementation would be fine.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30382/#review70431
-----------------------------------------------------------


On Jan. 30, 2015, 5:06 p.m., Christopher Tubbs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30382/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2015, 5:06 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-1844 and ACCUMULO-3514
>     https://issues.apache.org/jira/browse/ACCUMULO-1844
>     https://issues.apache.org/jira/browse/ACCUMULO-3514
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
>     ACCUMULO-3514 Use auto-service for start
>     
>     Use @AutoService annotations and Java's ServiceLoader mechanism to discover
>     classes which are executable by Accumulo's "start" jar with a keyword.
>     
>     This replaces manual intervention whenever we add a new option to the
>     bin/accumulo script and also auto-populates the usage for that script.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 5fc7a6e 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java f29efcc 
>   core/src/main/java/org/apache/accumulo/core/util/Classpath.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/CreateToken.java 79b241c 
>   core/src/main/java/org/apache/accumulo/core/util/Help.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/Jar.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/Version.java ee645ff 
>   minicluster/pom.xml ee6cdc8 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloRunner.java c45abc0

>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniClusterExecutable.java
PRE-CREATION 
>   pom.xml dda1cfe 
>   proxy/pom.xml 9312d7b 
>   proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 0a4d12e 
>   server/base/pom.xml c21a168 
>   server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 0a1f411 
>   server/base/src/main/java/org/apache/accumulo/server/util/Admin.java 77d5ea1 
>   server/base/src/main/java/org/apache/accumulo/server/util/Info.java 29fa135 
>   server/base/src/main/java/org/apache/accumulo/server/util/LoginProperties.java be5a7c8

>   server/base/src/main/java/org/apache/accumulo/server/util/ZooKeeperMain.java 0edcf71

>   server/gc/pom.xml 9602b95 
>   server/gc/src/main/java/org/apache/accumulo/gc/GCExecutable.java PRE-CREATION 
>   server/master/pom.xml 7e9ab1d 
>   server/master/src/main/java/org/apache/accumulo/master/MasterExecutable.java PRE-CREATION

>   server/monitor/pom.xml ba61aeb 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/MonitorExecutable.java PRE-CREATION

>   server/tracer/pom.xml ac9f45f 
>   server/tracer/src/main/java/org/apache/accumulo/tracer/TracerExecutable.java PRE-CREATION

>   server/tserver/pom.xml cd0f8ef 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TServerExecutable.java PRE-CREATION

>   shell/pom.xml db3530f 
>   shell/src/main/java/org/apache/accumulo/shell/Shell.java a64ff45 
>   start/src/main/java/org/apache/accumulo/start/Main.java c820883 
>   start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java PRE-CREATION

>   start/src/test/java/org/apache/accumulo/start/MainTest.java 1ea22fb 
>   test/src/test/java/org/apache/accumulo/start/KeywordStartIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/30382/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christopher Tubbs
> 
>


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