[GitHub] [geode] Bill commented on a change in pull request #5370: [WIP] GEODE-8298: Member version comparison sense inconsistent when deciding on multicast

[GitBox <gi...@apache.org>]

Bill commented on a change in pull request #5370:
URL: https://github.com/apache/geode/pull/5370#discussion_r454684275



##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +330,58 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  private void addSurpriseMemberWithVersion(Version version) {
+    MemberIdentifier surpriseMember = myMemberId;
+    surpriseMember.setVmViewId(3);
+    surpriseMember.setVersionObjectForTest(version);
+    manager.addSurpriseMember(surpriseMember);
+  }

Review comment:
       Please put `private` members after all `public` ones.

##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +330,58 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  private void addSurpriseMemberWithVersion(Version version) {
+    MemberIdentifier surpriseMember = myMemberId;
+    surpriseMember.setVmViewId(3);
+    surpriseMember.setVersionObjectForTest(version);
+    manager.addSurpriseMember(surpriseMember);
+  }
+
+  @Test
+  public void testMulticastWithOldVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = new MembershipView<>(myMemberId, 2, members);
+    addSurpriseMemberWithVersion(Version.GEODE_1_1_0);

Review comment:
       This test and the next two follow the same pattern. Can you factor out the common code?

##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -531,6 +515,12 @@ public void processView(long newViewId, MembershipView<ID> newView) {
     }
   }
 
+  protected boolean containsOldVersionMember(MembershipView<ID> newView,
+      VersionOrdinal referenceVersion) {
+    return Stream.concat(surpriseMembers.keySet().stream(), newView.getMembers().stream())
+        .anyMatch(member -> referenceVersion.isNewerThan(member.getVersionOrdinalObject()));
+  }

Review comment:
       yess

##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +330,58 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  private void addSurpriseMemberWithVersion(Version version) {
+    MemberIdentifier surpriseMember = myMemberId;
+    surpriseMember.setVmViewId(3);

Review comment:
       It is not clear from looking at this method that it is ok to mutate the object referenced by `myMemberId`. Off-hand it seems like that field refers to a member id representing the "local" member. So either way it seems wrong to modify it here to be the surprise member.
   
   I recommend you construct a new member id here for one-time use.

##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +330,58 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  private void addSurpriseMemberWithVersion(Version version) {
+    MemberIdentifier surpriseMember = myMemberId;
+    surpriseMember.setVmViewId(3);
+    surpriseMember.setVersionObjectForTest(version);
+    manager.addSurpriseMember(surpriseMember);
+  }
+
+  @Test
+  public void testMulticastWithOldVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = new MembershipView<>(myMemberId, 2, members);
+    addSurpriseMemberWithVersion(Version.GEODE_1_1_0);
+
+    assertTrue(manager.containsOldVersionMember(view, Version.CURRENT));
+  }
+
+  @Test
+  public void testMulticastWithCurrentVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = new MembershipView<>(myMemberId, 2, members);
+    addSurpriseMemberWithVersion(Version.CURRENT);
+
+    assertFalse(manager.containsOldVersionMember(view, Version.CURRENT));
+  }
+
+  @Test
+  public void testMulticastWithHigherVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = new MembershipView<>(myMemberId, 2, members);
+    addSurpriseMemberWithVersion(Version.CURRENT);
+
+    assertFalse(manager.containsOldVersionMember(view, Version.GEODE_1_1_0));
+  }
+
+  @Test
+  public void testMulticastWithOldVersionViewMember() {
+    members.get(0).setVersionObjectForTest(Version.GEODE_1_1_0);

Review comment:
       This test and the next two all follow the same pattern. Can you refactor out the commonality?

##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -359,7 +360,7 @@ boolean isDistributionMessage() {
   /**
    * Analyze a given view object, generate events as appropriate
    */
-  public void processView(long newViewId, MembershipView<ID> newView) {
+  public void processView(MembershipView<ID> newView) {

Review comment:
       great to get rid of the superfluous parameter!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org