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