----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51394/#review146723 -----------------------------------------------------------
I have a few review comments, and I think we need you to walk us through the algorithm and GMSEncrypt. We need to update documentation based on the current design. geode-core/src/main/java/com/gemstone/gemfire/distributed/ConfigurationProperties.java (line 1207) <https://reviews.apache.org/r/51394/#comment213304> I realize that SECURITY_CLIENT_DHALGO doesn't have much of a javadoc and you probably copied it to make this one, but you need to add a description of the property. This is the primary javadoc location for distribution config properties. geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/NetView.java (line 47) <https://reviews.apache.org/r/51394/#comment213308> How about fixing this TODO before you're done? Otherwise we're going to have another TODO story to deal with. geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/interfaces/JoinLeave.java (line 23) <https://reviews.apache.org/r/51394/#comment213309> remove reference to GMSMember from the interface geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/interfaces/Messenger.java (line 90) <https://reviews.apache.org/r/51394/#comment213339> These methods all need good javadocs and getPublickey should be getPublicKey with a capital "K". geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/locator/GMSLocator.java (line 257) <https://reviews.apache.org/r/51394/#comment213313> GMSLocator shouldn't directly refer to Messenger implementation classes. Can you add this to the Messenger interface and have JGroupsMessenger invoke the GMSEncrypt method? geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 878) <https://reviews.apache.org/r/51394/#comment213317> The "K" should be capitalized in this method name geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 879) <https://reviews.apache.org/r/51394/#comment213316> fix the TODO geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 897) <https://reviews.apache.org/r/51394/#comment213319> Please add a comment describing why a View from a still-valid member is being rejected here. geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 1134) <https://reviews.apache.org/r/51394/#comment213321> I don't see anything that changes in this message for each target member. Why aren't you just setting multiple recipients and sending it once? If it's due to some JGroupsMessenger requirement related to handshaking can't the messenger figure that out? geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 1232) <https://reviews.apache.org/r/51394/#comment213323> getPublickey should have a capital "K": getPublicKey geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2083) <https://reviews.apache.org/r/51394/#comment213325> Make this debug level? geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2229) <https://reviews.apache.org/r/51394/#comment213327> delete dead code geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java (line 2358) <https://reviews.apache.org/r/51394/#comment213328> delete dead code geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/GMSEncrypt.java (line 1) <https://reviews.apache.org/r/51394/#comment213329> Let's walk through this together geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/JGroupsMessenger.java (line 725) <https://reviews.apache.org/r/51394/#comment213333> why is this using an Iterator? That's taking a step backward and I can't see why it's needed. geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/JGroupsMessenger.java (line 743) <https://reviews.apache.org/r/51394/#comment213334> unnecessary cast geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/JGroupsMessenger.java (line 898) <https://reviews.apache.org/r/51394/#comment213336> It would be a lot nicer and somewhat faster to add an interface like MessageWithRequestID and have the message classes implement it. - Bruce Schuchardt On Aug. 24, 2016, 10:10 p.m., Hitesh Khamesra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51394/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2016, 10:10 p.m.) > > > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer. > > > Repository: geode > > > Description > ------- > > running precheckin.. > > > Diffs > ----- > > .gitignore 1b9dd0a > > geode-core/src/main/java/com/gemstone/gemfire/distributed/ConfigurationProperties.java > 5c3a282 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/AbstractDistributionConfig.java > cc544f6 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/DMStats.java > 1b36ee1 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionConfig.java > 2ff6540 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionConfigImpl.java > fd4743b > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionStats.java > dcd4aba > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/InternalLocator.java > 3c9fa97 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/LonerDistributionManager.java > 7f8eed6 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/InternalDistributedMember.java > 2d8b8e1 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/MemberFactory.java > 013ef35 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/MemberServices.java > 7f48e46 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/NetView.java > f3c04f7 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/GMSMember.java > d5d0b8e > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/GMSMemberFactory.java > 4328bed > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/interfaces/JoinLeave.java > 87409c5 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/interfaces/Messenger.java > 5bb6c4b > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/locator/FindCoordinatorRequest.java > 5c0a1d1 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/locator/FindCoordinatorResponse.java > 0427cb4 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/locator/GMSLocator.java > 1065214 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeave.java > 58b794a > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messages/InstallViewMessage.java > c41584f > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messages/JoinRequestMessage.java > 0e84e92 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messages/JoinResponseMessage.java > ad9c319 > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/GMSEncrypt.java > PRE-CREATION > > geode-core/src/main/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/JGroupsMessenger.java > a119bb5 > > geode-core/src/main/java/com/gemstone/gemfire/internal/InternalDataSerializer.java > 04770bb > > geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java > 95bf700 > > geode-core/src/test/java/com/gemstone/gemfire/cache30/DistributedMulticastRegionDUnitTest.java > 3c05794 > > geode-core/src/test/java/com/gemstone/gemfire/cache30/DistributedMulticastRegionWithUDPSecurityDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/cache30/ReconnectDUnitTest.java > 59fc206 > > geode-core/src/test/java/com/gemstone/gemfire/cache30/ReconnectWithUDPSecurityDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/distributed/LocatorDUnitTest.java > 954846f > > geode-core/src/test/java/com/gemstone/gemfire/distributed/LocatorUDPSecurityDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/DistributionConfigJUnitTest.java > 6d6f36d > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/MembershipJUnitTest.java > 9c7180e > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/GMSMemberJUnitTest.java > 7eef594 > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/locator/GMSLocatorRecoveryJUnitTest.java > fcf77a0 > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java > ec639aa > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/GMSEncryptJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/distributed/internal/membership/gms/messenger/JGroupsMessengerJUnitTest.java > 58dbe48 > > geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedDataSerializables.txt > 5172709 > gradle/rat.gradle 0ce82b9 > > Diff: https://reviews.apache.org/r/51394/diff/ > > > Testing > ------- > > > Thanks, > > Hitesh Khamesra > >
