accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 17947: ACCUMULO-841 - refactor of randomwalk State
Date Tue, 11 Feb 2014 16:41:00 GMT


> On Feb. 11, 2014, 11:01 a.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.

Much better names. Will do.


> On Feb. 11, 2014, 11:01 a.m., Eric Newton wrote:
> > test/src/main/java/org/apache/accumulo/test/randomwalk/ConfigProperties.java, line
23
> > <https://reviews.apache.org/r/17947/diff/1/?file=482585#file482585line23>
> >
> >     The comment is wrong.  This class doesn't create objects: it can find the username
and password.  Is it worth creating a whole class that just knows KEY_USERNAME and KEY_PASSWORD?

The comment is left over from the refactoring process. Thanks for pointing that out, I'll
fix it.

I see ConfigProperties as specifically holding the unchanging properties configured through
randomwalk.conf, vs. the test-specific, dynamic state information that State should hold.
In a more thorough refactoring, I'd have tests use State and ConfigProperties separately.


> On Feb. 11, 2014, 11:01 a.m., Eric Newton wrote:
> > test/src/main/java/org/apache/accumulo/test/randomwalk/ConfigProperties.java, line
43
> > <https://reviews.apache.org/r/17947/diff/1/?file=482585#file482585line43>
> >
> >     Can we use the fancy guava class to check for preconditions?

Yeah, I'm not sure why I didn't just go for it. I'll switch to it.


- Bill


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


On Feb. 11, 2014, 10:24 a.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17947/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 10:24 a.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