accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request 17947: ACCUMULO-841 - refactor of randomwalk State
Date Tue, 11 Feb 2014 16:42:00 GMT


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


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