You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Scott Jewell <sj...@pivotal.io> on 2016/05/02 18:26:07 UTC
Re: Review Request 46854: GEODE-1224: Modify
BucketRegion.getCloningEnabled
to call this.partitionedRegion.getCloningEnabled
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46854/
-----------------------------------------------------------
(Updated May 2, 2016, 4:26 p.m.)
Review request for geode, Darrel Schneider and Ken Howe.
Repository: geode
Description
-------
Modified BucketRegion and PRDeltaPropagationDUnitTest
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java d37f025d9dc81b938425c277f33b7138951d2252
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/BucketRegion.java f5ae0fb37c9347d5ce8fe18bc1d61fdace941c98
geode-core/src/test/java/com/gemstone/gemfire/DeltaTestImpl.java cd824590ada9652db75f4dd94cd2c0dba3cf9b5a
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java e8816f979f1b1627f7540975a7904065735c016d
Diff: https://reviews.apache.org/r/46854/diff/
Testing (updated)
-------
precheckin
Thanks,
Scott Jewell
Re: Review Request 46854: GEODE-1224: Modify
BucketRegion.getCloningEnabled
to call this.partitionedRegion.getCloningEnabled
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46854/#review131341
-----------------------------------------------------------
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (lines 151 - 152)
<https://reviews.apache.org/r/46854/#comment195272>
See comment re: createDeltaPR refactoring. This should not need to be changed
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (line 318)
<https://reviews.apache.org/r/46854/#comment195271>
See comment re: createDeltaPR refactoring. This should not need to be changed.
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (lines 840 - 843)
<https://reviews.apache.org/r/46854/#comment195269>
As we discussed, I found this refactoring of creatDeltaPR (and createPR) by adding a second Boolean argument to be confusing. All existing calls need to be changed to add the new agrument, and seeing soemthing like createDeltaPR(false, true) when reading the code leaves you wondering what the arguments signify.
I would have left the original method signatures as-is and added new methods, createDeltaPRWithCloning and createPRWithCloning. For imnplementation, all four methods (createDeltaPR, createDeltaPRWithCloning, createPR, \u2021 createrPRWithCloning) could all call a common private method, createPR, that has both Boolean arguments as in your new version of createPR.
In summary, the signatures for this family of methods would be (where implementation of the first 4 would all call the last one):
private static void createDeltaPR(Boolean)
private static void createDeltaPRWithCloning(Boolean, Boolean)
public static void createDeltaPR(Boolean)
public static void createDeltaPRWithCloning(Boolean)
private static void createDeltaPR(Boolean, Boolean)
This is one way to clarify the code, see what other reviewers think.
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (lines 884 - 886)
<https://reviews.apache.org/r/46854/#comment195270>
See comment re: createDeltaPR refactoring. This call should not need to change
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (line 1089)
<https://reviews.apache.org/r/46854/#comment195273>
Why the argument type change from PRDeltaTestImpl to DeltaTestImpl?
- Ken Howe
On May 2, 2016, 4:26 p.m., Scott Jewell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46854/
> -----------------------------------------------------------
>
> (Updated May 2, 2016, 4:26 p.m.)
>
>
> Review request for geode, Darrel Schneider and Ken Howe.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Modified BucketRegion and PRDeltaPropagationDUnitTest
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java d37f025d9dc81b938425c277f33b7138951d2252
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/BucketRegion.java f5ae0fb37c9347d5ce8fe18bc1d61fdace941c98
> geode-core/src/test/java/com/gemstone/gemfire/DeltaTestImpl.java cd824590ada9652db75f4dd94cd2c0dba3cf9b5a
> geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java e8816f979f1b1627f7540975a7904065735c016d
>
> Diff: https://reviews.apache.org/r/46854/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> Scott Jewell
>
>
Re: Review Request 46854: GEODE-1224: Modify
BucketRegion.getCloningEnabled
to call this.partitionedRegion.getCloningEnabled
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46854/#review131337
-----------------------------------------------------------
geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java (line 891)
<https://reviews.apache.org/r/46854/#comment195267>
Do this fix with its own ticket. We may have existing flaky test CI tickets on this test case so it could be fixed on one of those.
If not then file a new ticket that just says this test should set the server port to 0 instead of using getRandomAvailablePort.
- Darrel Schneider
On May 2, 2016, 9:26 a.m., Scott Jewell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46854/
> -----------------------------------------------------------
>
> (Updated May 2, 2016, 9:26 a.m.)
>
>
> Review request for geode, Darrel Schneider and Ken Howe.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Modified BucketRegion and PRDeltaPropagationDUnitTest
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java d37f025d9dc81b938425c277f33b7138951d2252
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/BucketRegion.java f5ae0fb37c9347d5ce8fe18bc1d61fdace941c98
> geode-core/src/test/java/com/gemstone/gemfire/DeltaTestImpl.java cd824590ada9652db75f4dd94cd2c0dba3cf9b5a
> geode-cq/src/test/java/com/gemstone/gemfire/internal/cache/PRDeltaPropagationDUnitTest.java e8816f979f1b1627f7540975a7904065735c016d
>
> Diff: https://reviews.apache.org/r/46854/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> Scott Jewell
>
>