You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Josh Elser <jo...@gmail.com> on 2014/10/20 18:28:30 UTC
Review Request 26943: Changes to ZK for implicit retry handling
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-3242
https://issues.apache.org/jira/browse/ACCUMULO-3242
Repository: accumulo
Description
-------
We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
Diffs
-----
fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
Diff: https://reviews.apache.org/r/26943/diff/
Testing
-------
UTs/ITs
Thanks,
Josh Elser
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 20, 2014, 7 p.m., Eric Newton wrote:
> >
Thanks, Eric. I just took care of these.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57379
-----------------------------------------------------------
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by Eric Newton <er...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57379
-----------------------------------------------------------
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
<https://reviews.apache.org/r/26943/#comment98067>
nit: spelling "wil"
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java
<https://reviews.apache.org/r/26943/#comment98092>
maybe add hashCode and equals?
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java
<https://reviews.apache.org/r/26943/#comment98093>
nit: braces
- Eric Newton
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 21, 2014, 4:21 p.m., kturner wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java, line 167
> > <https://reviews.apache.org/r/26943/diff/1/?file=725844#file725844line167>
> >
> > why avoid sleeping before retry in this case?
The sleep happens down below in the call to wait()
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57590
-----------------------------------------------------------
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 21, 2014, 4:21 p.m., kturner wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java, line 167
> > <https://reviews.apache.org/r/26943/diff/1/?file=725844#file725844line167>
> >
> > why avoid sleeping before retry in this case?
>
> Josh Elser wrote:
> The sleep happens down below in the call to wait()
Ooops, you're right. I goofed that.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57590
-----------------------------------------------------------
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57590
-----------------------------------------------------------
fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
<https://reviews.apache.org/r/26943/#comment98370>
why avoid sleeping before retry in this case?
- kturner
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26943: Changes to ZK for implicit retry handling
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26943/#review57661
-----------------------------------------------------------
fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java
<https://reviews.apache.org/r/26943/#comment98472>
this test would be a bit more realistic if getData returned different data on the 2nd call AND the mutation was a function of the input. I suppose the times(2) is most of the way there. However it does not verfy the expected order AFAICT :
getData
setData
getData
setData
Also does not verify that data from 2nd call to getData is used (because getData always returns 3 I think). If there was a bug in the code where it reused the what the 1st call to getData returned, this test would not catch it.
fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java
<https://reviews.apache.org/r/26943/#comment98469>
misleading comment
- kturner
On Oct. 20, 2014, 4:28 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26943/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3242
> https://issues.apache.org/jira/browse/ACCUMULO-3242
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> We didn't do some things correctly WRT zookeeper retries. Attempt to make it better. Made a review in case people want to give some feedback
>
>
> Diffs
> -----
>
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReaderWriter.java afc2250
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java 4a37172
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java 3fcb738
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java d9eb243
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java 994f395
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java b29b88a
> fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java e0d8831
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java PRE-CREATION
> fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriterTest.java PRE-CREATION
> server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 5cbffc3
> server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java 3cac2e9
>
> Diff: https://reviews.apache.org/r/26943/diff/
>
>
> Testing
> -------
>
> UTs/ITs
>
>
> Thanks,
>
> Josh Elser
>
>