Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 2A80017A39 for ; Sat, 31 Jan 2015 07:21:25 +0000 (UTC) Received: (qmail 74166 invoked by uid 500); 31 Jan 2015 07:21:25 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 74123 invoked by uid 500); 31 Jan 2015 07:21:25 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 74102 invoked by uid 99); 31 Jan 2015 07:21:25 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 31 Jan 2015 07:21:25 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 4365C1C6F22; Sat, 31 Jan 2015 07:21:29 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3199378783843306561==" MIME-Version: 1.0 Subject: Re: Review Request 30382: ACCUMULO-3514 Use auto service for start From: "Christopher Tubbs" To: "Mike Drob" , "Christopher Tubbs" , "accumulo" Date: Sat, 31 Jan 2015 07:21:29 -0000 Message-ID: <20150131072129.1308.89775@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Christopher Tubbs" X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/30382/ X-Sender: "Christopher Tubbs" References: <20150130224049.1308.67263@reviews.apache.org> In-Reply-To: <20150130224049.1308.67263@reviews.apache.org> Reply-To: "Christopher Tubbs" X-ReviewRequest-Repository: accumulo --===============3199378783843306561== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Jan. 30, 2015, 5:40 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/util/Classpath.java, line 33 > > > > > > 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 > > > > > > 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 > > > > > > 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 > > --===============3199378783843306561==--