You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Lynn Gallinat <lg...@pivotal.io> on 2017/09/07 16:56:46 UTC
Review Request 62164: GEODE-3566 Moving a bucket during rebalancing
does not update overflow stats
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
Repository: geode
Description
-------
We now update the stats in a member that lost a bucket due to rebalancing
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/62164/diff/1/
Testing
-------
Precheckin
Thanks,
Lynn Gallinat
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Nick Reich <nr...@pivotal.io>.
> On Sept. 7, 2017, 8:08 p.m., Nick Reich wrote:
> > geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/62164/diff/1/?file=1817801#file1817801line17>
> >
> > I think this is the wrong library, or are you using assertj on purpose instead of junit?
>
> Lynn Gallinat wrote:
> The use of assertj was intentional and recommended to me by Kirk.
Ok. I just had not seen any tests yet that used assertj here before, so was unsure if that was a new preferred/acceptable library or not.
- Nick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184872
-----------------------------------------------------------
On Sept. 8, 2017, 4:10 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 4:10 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/2/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Lynn Gallinat <lg...@pivotal.io>.
> On Sept. 7, 2017, 8:08 p.m., Nick Reich wrote:
> > geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/62164/diff/1/?file=1817801#file1817801line17>
> >
> > I think this is the wrong library, or are you using assertj on purpose instead of junit?
The use of assertj was intentional and recommended to me by Kirk.
> On Sept. 7, 2017, 8:08 p.m., Nick Reich wrote:
> > geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
> > Lines 128 (patched)
> > <https://reviews.apache.org/r/62164/diff/1/?file=1817801#file1817801line128>
> >
> > This could be cleaned up by getting the region once at the top of the method and using that variable throughout.
The region does not exist in the process that is executing validateInitialRegion so I can't get it at the top of the method. The lambdas are being executed in different members, so each one needs to get the region instance specific to that member.
- Lynn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184872
-----------------------------------------------------------
On Sept. 8, 2017, 4:10 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 4:10 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/2/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184872
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 17 (patched)
<https://reviews.apache.org/r/62164/#comment261066>
I think this is the wrong library, or are you using assertj on purpose instead of junit?
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 128 (patched)
<https://reviews.apache.org/r/62164/#comment261065>
This could be cleaned up by getting the region once at the top of the method and using that variable throughout.
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 129 (patched)
<https://reviews.apache.org/r/62164/#comment261069>
This method seems to be doing two distinct things: the first is a vm specific check of region entry and bucket count sizes (which could be its own method that takes only a single vm) and the second is checking overflow, which could also be its own method (especially since only one region is checked).
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 156 (patched)
<https://reviews.apache.org/r/62164/#comment261070>
Normalize variable names to camel case?
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 249 (patched)
<https://reviews.apache.org/r/62164/#comment261067>
use TEST_REGION constant
- Nick Reich
On Sept. 7, 2017, 4:56 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 4:56 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/1/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184876
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Sept. 7, 2017, 9:56 a.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 9:56 a.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/1/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184842
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
Lines 2460 (patched)
<https://reviews.apache.org/r/62164/#comment261032>
Can remove nesting by checking dr != null && destroyDiskRegion
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java
Lines 91 (patched)
<https://reviews.apache.org/r/62164/#comment261064>
It does not look like the second part of this method uses the shortcut or overflow parameters, so it could be a seperate method. Since this method does two different things, that would probably be optimal.
- Nick Reich
On Sept. 7, 2017, 4:56 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 4:56 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/1/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Nick Reich <nr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184982
-----------------------------------------------------------
Ship it!
Ship It!
- Nick Reich
On Sept. 8, 2017, 4:10 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 4:10 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/2/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by Lynn Gallinat <lg...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/
-----------------------------------------------------------
(Updated Sept. 8, 2017, 4:10 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
Changes
-------
Changes in response to review.
Repository: geode
Description
-------
We now update the stats in a member that lost a bucket due to rebalancing
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/62164/diff/2/
Changes: https://reviews.apache.org/r/62164/diff/1-2/
Testing
-------
Precheckin
Thanks,
Lynn Gallinat
Re: Review Request 62164: GEODE-3566 Moving a bucket during
rebalancing does not update overflow stats
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62164/#review184863
-----------------------------------------------------------
Ship it!
Ship It!
- anilkumar gingade
On Sept. 7, 2017, 4:56 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62164/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 4:56 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, and Nick Reich.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We now update the stats in a member that lost a bucket due to rebalancing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java 465a3dd
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/BucketRebalanceStatRegressionTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62164/diff/1/
>
>
> Testing
> -------
>
> Precheckin
>
>
> Thanks,
>
> Lynn Gallinat
>
>