You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/07/14 21:31:20 UTC

[GitHub] [geode] kamilla1201 opened a new pull request #5370: [WIP] GEODE-8298: Member version comparison sense inconsistent when deciding on multicast

kamilla1201 opened a new pull request #5370:
URL: https://github.com/apache/geode/pull/5370


   [GEODE-8298](https://issues.apache.org/jira/browse/GEODE-8298)
   These changes fix inconsistent version comparison in `GMSMembership.processView()`.
   
   The goal of the comparison is to find the oldest Geode version and if that version is older than the local version, disable multicast. The code responsible for this was extracted from `processView` to a new method `containsOldVersionMember` to allow for better unit testing.
   
   Thank you for submitting a contribution to Apache Geode.
   
   In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken:
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   - [ ] Does `gradlew build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse for build issues and
   submit an update to your PR as soon as possible. If you need help, please send an
   email to dev@geode.apache.org.
   


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

[GitHub] [geode] Bill merged pull request #5370: GEODE-8298: Member version comparison sense inconsistent when deciding on multicast

Posted by GitBox <gi...@apache.org>.
Bill merged pull request #5370:
URL: https://github.com/apache/geode/pull/5370


   


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

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

Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5370:
URL: https://github.com/apache/geode/pull/5370#discussion_r455891914



##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
##########
@@ -507,8 +507,8 @@ public void setVmKind(int vmKind) {
 
 
   @Override
-  public void setVersion(Version v) {
-    setVersionOrdinal(v.ordinal());
+  public void setVersion(VersionOrdinal versionOrdinal) {
+    setVersionOrdinal(versionOrdinal.ordinal());

Review comment:
       Let's set `versionOrdinal` (object reference) directly here and eliminate `MemberData.setVersionOrdinal(short)` entirely. The method shouldn't exist at all. We should not be passing a `short` ordinal to a `MemberData`.
   
   There are three places `MemberData.setVersionOrdinal(short)` is called:
   1. here: just set the field directly
   2. `readEssentialData()`: replace `setVersionOrdinal(VersioningIO.readOrdinal(in));` with `setVersion(Versioning. getVersionOrdinal(VersioningIO.readOrdinal(in)))`
   3. `GMSMemberDataVersionJUnitTest.testSetVersionOrdinal()`: delete this test method




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

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

Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5370:
URL: https://github.com/apache/geode/pull/5370#discussion_r455147203



##########
File path: geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
##########
@@ -328,4 +332,63 @@ public void noDispatchWhenSick() throws MemberShunnedException, MemberStartupExc
     assertThat(spy.getStartupEvents()).isEmpty();
   }
 
+  @Test
+  public void testIsMulticastAllowedWithOldVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+    manager.addSurpriseMember(createSurpriseMember(OLDER_THAN_CURRENT_VERSION));
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+  }
+
+  @Test
+  public void testIsMulticastAllowedWithCurrentVersionSurpriseMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+    manager.addSurpriseMember(createSurpriseMember(Version.CURRENT));
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+  }
+
+  @Test
+  public void testIsMulticastAllowedWithOldVersionViewMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(OLDER_THAN_CURRENT_VERSION);
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+  }
+
+  @Test
+  public void testMulticastAllowedWithCurrentVersionViewMember() {
+    MembershipView<MemberIdentifier> view = createMembershipView(Version.CURRENT);
+
+    manager.processView(view);
+
+    assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+  }
+
+  private MemberIdentifier createSurpriseMember(Version version) {
+    MemberIdentifier surpriseMember = createMemberID(DEFAULT_PORT + 5);
+    surpriseMember.setVmViewId(3);
+    surpriseMember.setVersionObjectForTest(version);
+    return surpriseMember;
+  }
+
+  private MembershipView<MemberIdentifier> createMembershipView(Version version) {
+    List<MemberIdentifier> viewMembers = createMemberIdentifiers(version);
+    return new MembershipView<>(myMemberId, 2, viewMembers);
+  }
+
+  private List<MemberIdentifier> createMemberIdentifiers(Version memberVersion) {
+    List<MemberIdentifier> viewMembers = new ArrayList<>();
+    for (int i = 0; i < 2; ++i) {
+      MemberIdentifier memberIdentifier = createMemberID(DEFAULT_PORT + 6 + i);
+      memberIdentifier.setVersionObjectForTest(memberVersion);

Review comment:
       A small point: this routine will set _every_ member's version to `memberVersion`. I think it would be a better test if only _one_ member's version was set to `memberVersion` and the rest were set to `Version.CURRENT`.




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

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

Posted by 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