Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============7552887090698821586=="
MIME-Version: 1.0
Subject: Re: Review Request 17947: ACCUMULO-841 - refactor of randomwalk State
From: keith@deenlo.com
To: "Eric Newton" <eric.newton@gmail.com>,
 "Bill Havanki" <bhavanki@clouderagovt.com>,
 "accumulo" <dev@accumulo.apache.org>, keith@deenlo.com
Date: Tue, 11 Feb 2014 16:42:00 -0000
Message-ID: <20140211164200.29246.62927@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: noreply@reviews.apache.org
References: <20140211160113.6237.41088@reviews.apache.org>
In-Reply-To: <20140211160113.6237.41088@reviews.apache.org>
Reply-To: keith@deenlo.com

--===============7552887090698821586==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On Feb. 11, 2014, 4:01 p.m., Eric Newton wrote:
> > test/src/main/java/org/apache/accumulo/test/randomwalk/ConfigObjectManager.java, line 39
> > <https://reviews.apache.org/r/17947/diff/1/?file=482584#file482584line39>
> >
> >     Almost every class can be considered an ObjectManager.  Consider renaming the class ClientSideObjects or ClientObjects.
> 
> Bill Havanki wrote:
>     Much better names. Will do.

Naming stuff sucks.  I was thinking about what the goal of this refactoring is.   One reason is to make it easier for someone to write random walk test.   Looking at the existing code I think one possible source of confusion is that methods intended for use by the framework itself are public.  Someone trying to write RW test has to spend time figuring this out so they can ignore those methods.  

Maybe rename ClientObjectManager to TestEnvironment (I think this communicates its purpose better) and add the username and password methods to it.  Looking at the existing getProperties method on State, its only used by the framework code and holds config for the framework.   Could possibly make getProperties() package private and add javadoc about it returning framework config.   There are some other things in state that are not used by test and could be made package private.


- kturner


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


On Feb. 11, 2014, 3:24 p.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17947/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 3:24 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-841
>     https://issues.apache.org/jira/browse/ACCUMULO-841
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> This is a refactoring of the State class used for randomwalk. I tried to balance the extent of the refactoring with the extent of changes to the tests themselves. So, State is still a central class, but it delegates to other more focused classes for almost everything beyond storing just state key/value pairs. Other notes:
> 
> - The node visiting mechanism was unused so it's removed. Module has a maxHops counting mechanism which does the same thing.
> - The getCredentials() method was only ever used to then get an authentication token, so the tests now get the token directly, and getCredentials() is gone.
> 
> 
> Diffs
> -----
> 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/ConfigObjectManager.java PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/ConfigProperties.java PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/Module.java 25684809daf29644550c05256c7abaf60fc2e75c 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/Node.java 1868adedcbb41040d2b0e2e2edff642799f10673 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/State.java f4102ab5dbff63e18ba5673126eeb5e646b84771 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/multitable/CopyTable.java 5a2172d6eddf302fb266c82e174ab235358e9cd6 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/security/AlterTablePerm.java 335ae31dc05ebade9f30187a2ab4d9c79ae205c6 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/security/SecurityFixture.java 30f12fbc57b625489271a3a4a23f8c21b4df9ec1 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/security/SetAuths.java 54bc34aface7428c3149ffbc438cc0be82cee72f 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/security/Validate.java 03718ce4e044e985cb309c01c531630cf9222a33 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/sequential/MapRedVerify.java f60fe44c66a6f23242aa442f279ecb86c5162109 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/shard/BulkInsert.java 41acce2575de571fd48fdf5759455be3c5698e33 
> 
> Diff: https://reviews.apache.org/r/17947/diff/
> 
> 
> Testing
> -------
> 
> Compile works; ran MultiTable randomwalk, which does run MR jobs to test getMapReduceJars(); also ran Security randomwalk.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>


--===============7552887090698821586==--