----------------------------------------------------------- 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, ‡ 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 > >
