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 DA4AC105F5 for ; Wed, 6 Nov 2013 19:49:10 +0000 (UTC) Received: (qmail 30757 invoked by uid 500); 6 Nov 2013 19:49:10 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 30689 invoked by uid 500); 6 Nov 2013 19:49:10 -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 30681 invoked by uid 500); 6 Nov 2013 19:49:10 -0000 Delivered-To: apmail-incubator-accumulo-dev@incubator.apache.org Received: (qmail 30676 invoked by uid 99); 6 Nov 2013 19:49:10 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Nov 2013 19:49:10 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 9B7F81D3905; Wed, 6 Nov 2013 19:49:07 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============4820897774263013065==" MIME-Version: 1.0 Subject: Re: Review Request 15279: ACCUMULO-1556: Clarify initialization error messages for pre-initialized filesystem From: "Sean Busbey" To: "Bill Havanki" , "Sean Busbey" , "accumulo" Date: Wed, 06 Nov 2013 19:49:07 -0000 Message-ID: <20131106194907.30021.65587@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Sean Busbey" X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/15279/ X-Sender: "Sean Busbey" References: <20131106190203.30021.19399@reviews.apache.org> In-Reply-To: <20131106190203.30021.19399@reviews.apache.org> Reply-To: "Sean Busbey" --===============4820897774263013065== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15279/#review28290 ----------------------------------------------------------- src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java please don't include formatting fixes unrelatd to the change. src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java please don't include formatting fixes unrelated to the issue. src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java Does this buy us much beyond just letting the original IOException propagate without wrapping? src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java Include a note that this test class is not threadsafe. The default poms and build scripts don't run parallel unit tests. But I know the build speed is something we care about, so we'll get there eventually. Best to have a warning for future maintainers. How much detail to include is up to you. It won't work with JUnit 4.7+'s parallel, since they're in the same JVM: http://maven.apache.org/surefire/maven-surefire-plugin/examples/junit.html#Running_tests_in_parallel It will work with Surefire's fork and reuseForks=false, since that's a JVM per class http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html#Forked_Test_Execution src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java Should have an @After that restores Initialize.setZooReaderWriter src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java Can you add a test that confirms when the underlying filesystem throws an exception on the exists call we get an IOException back out of checkInit? - Sean Busbey On Nov. 6, 2013, 7:02 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15279/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2013, 7:02 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-1556 > https://issues.apache.org/jira/browse/ACCUMULO-1556 > > > Repository: accumulo > > > Description > ------- > > The Initialize class now generates clearer error messages if an initialized instance is discovered. The messages vary depending on whether instance.dfs.uri is used. > > Note that to facilitate unit testing, the verification logic in Initialize.doInit() was refactored into a checkInit() method. > > > Diffs > ----- > > pom.xml 9ed2fdf1c7a1f8831667b27bfaa307fbe2467fe8 > src/server/pom.xml 6421bc69cda5116f9716d30adc3d510baa3bb7d6 > src/server/src/main/java/org/apache/accumulo/server/util/Initialize.java 51576fcc8ff8ffcd65a78bfeaaaf51036360a6bc > src/server/src/test/java/org/apache/accumulo/server/util/InitializeTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/15279/diff/ > > > Testing > ------- > > Ran initialization with patch changes on 1.4.3 cluster under CDH 4.3. Tested successful initialization and correct emission of error messages when instance.dfs.uri was used and was not used. Also, implemented unit tests for altered code. > > > Thanks, > > Bill Havanki > > --===============4820897774263013065==--