You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Sai Boorlagadda <sb...@pivotal.io> on 2016/04/04 21:23:23 UTC
Review Request 45700: Fixed BucketOperatorWrapper to invoke onFailure
if bucket creation fails
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/
-----------------------------------------------------------
Review request for geode, Darrel Schneider and Dan Smith.
Bugs: GEODE-1153
https://issues.apache.org/jira/browse/GEODE-1153
Repository: geode
Description
-------
* Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
* Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
* Added JUnit tests to test these higher level classes
* Added a DUnit to test the overall redundancy if there is any failure in create bucket
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45700/diff/
Testing
-------
Thanks,
Sai Boorlagadda
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review127221
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Smith
On April 4, 2016, 10:32 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 10:32 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review126962
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On April 4, 2016, 3:32 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 3:32 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review129984
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Smith
On April 21, 2016, 7:53 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 7:53 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/PartitionedRegionLoadModel.java e0240113130817aff896ed0467aca12ea8396da0
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Sai Boorlagadda <sb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review129947
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/PartitionedRegionLoadModel.java (line 400)
<https://reviews.apache.org/r/45700/#comment193498>
I am not sure if this is rare.
Because the current code takes care of case {B(0), B1(0), B2(1), B3(2)} where it first prioritizes reaching redundancy level of B0 & B1 to atleast the level of B2 and then satify all these three before joining with B3.
So may be rather than delaying to onSuccess, consider removing from the set before mutating in onFailure?
- Sai Boorlagadda
On April 21, 2016, 7:53 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 7:53 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/PartitionedRegionLoadModel.java e0240113130817aff896ed0467aca12ea8396da0
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review129970
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On April 21, 2016, 12:53 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 12:53 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/PartitionedRegionLoadModel.java e0240113130817aff896ed0467aca12ea8396da0
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Sai Boorlagadda <sb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/
-----------------------------------------------------------
(Updated April 21, 2016, 7:53 p.m.)
Review request for geode, Darrel Schneider and Dan Smith.
Changes
-------
Deferred redundancy check to see if further redendancy have to satisfy and adding to the set to onSuccess.
Bugs: GEODE-1153
https://issues.apache.org/jira/browse/GEODE-1153
Repository: geode
Description
-------
* Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
* Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
* Added JUnit tests to test these higher level classes
* Added a DUnit to test the overall redundancy if there is any failure in create bucket
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/PartitionedRegionLoadModel.java e0240113130817aff896ed0467aca12ea8396da0
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45700/diff/
Testing
-------
Thanks,
Sai Boorlagadda
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Sai Boorlagadda <sb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/
-----------------------------------------------------------
(Updated April 4, 2016, 10:32 p.m.)
Review request for geode, Darrel Schneider and Dan Smith.
Changes
-------
Thansk Dan for the review. I agree we should think twice before power mocking.
Bugs: GEODE-1153
https://issues.apache.org/jira/browse/GEODE-1153
Repository: geode
Description
-------
* Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
* Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
* Added JUnit tests to test these higher level classes
* Added a DUnit to test the overall redundancy if there is any failure in create bucket
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45700/diff/
Testing
-------
Thanks,
Sai Boorlagadda
Re: Review Request 45700: Fixed BucketOperatorWrapper to invoke
onFailure if bucket creation fails
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45700/#review126944
-----------------------------------------------------------
Great job adding a whole bunch more tests for these (previously) inner classes!
I had a couple of questions below, related the use of powermock. IMHO needing powermock is a sign that we should just change the code so we don't need it anymore.
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java (line 57)
<https://reviews.apache.org/r/45700/#comment190021>
Why is it necessary to use powermock here? Why not use InternalResourceManager.setResourceObserver?
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java (line 74)
<https://reviews.apache.org/r/45700/#comment190022>
Instead of using powermock here, would it make more sense to change the code to make moveBucket an instance method on PRRebalanceOp, and then just mock that class?
- Dan Smith
On April 4, 2016, 7:23 p.m., Sai Boorlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45700/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 7:23 p.m.)
>
>
> Review request for geode, Darrel Schneider and Dan Smith.
>
>
> Bugs: GEODE-1153
> https://issues.apache.org/jira/browse/GEODE-1153
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Fixed invoking completion callback after delegate finishes createdBucket and invokes wrapper`s completion callback
> * Promoted BucketOperatorWrapper & BucketOperatorImpl to be higher level classes for unit testing callback invoacation
> * Added JUnit tests to test these higher level classes
> * Added a DUnit to test the overall redundancy if there is any failure in create bucket
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PartitionedRegionRebalanceOp.java 864287624d0ec6db0a9e5170360a5e6fbda5d26e
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImpl.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapper.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/control/RebalanceOperationDUnitTest.java 26ebc1679c3b0502e0bf3577366ff81697e89c09
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorImplTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/rebalance/BucketOperatorWrapperTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45700/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sai Boorlagadda
>
>