[
https://issues.apache.org/jira/browse/GEODE-8298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17157742#comment-17157742
]
ASF GitHub Bot commented on GEODE-8298:
---------------------------------------
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:
us...@infra.apache.org
> member version comparison sense inconsistent when deciding on multicast
> -----------------------------------------------------------------------
>
> Key: GEODE-8298
> URL: https://issues.apache.org/jira/browse/GEODE-8298
> Project: Geode
> Issue Type: Bug
> Components: membership
> Reporter: Bill Burcham
> Assignee: Kamilla Aslami
> Priority: Major
> Labels: starter
>
> Since about 2014 when we introduced the {{Version}} class to replace use of
> {{short}} s all over the place for serialization versions, these two loops in
> {{GMSMembership.processView()}} have used comparisons that disagree in sense:
> {code}
> // We perform the update under a global lock so that other
> // incoming events will not be lost in terms of our global view.
> latestViewWriteLock.lock();
> try {
> // first determine the version for multicast message serialization
> VersionOrdinal version = Version.CURRENT;
> for (final Entry<ID, Long> internalIDLongEntry : surpriseMembers
> .entrySet()) {
> ID mbr = internalIDLongEntry.getKey();
> final VersionOrdinal itsVersion = mbr.getVersionObject();
> if (itsVersion != null && version.compareTo(itsVersion) < 0) {
> version = itsVersion;
> }
> }
> for (ID mbr : newView.getMembers()) {
> final VersionOrdinal itsVersion = mbr.getVersionObject();
> if (itsVersion != null && itsVersion.compareTo(version) < 0) {
> version = mbr.getVersionObject();
> }
> }
> disableMulticastForRollingUpgrade = !version.equals(Version.CURRENT);
> {code}
> The goal here is to find the oldest version and if that version is older than
> our local version we disable multicast. So we want to put the minimum into
> {{version}}. So the first loop's comparison is wrong and the second one is
> right.
> While we are in here let's combine the two loops using
> {{Stream.concat(surpriseMembers.entrySet().stream().map(entry->entry.getKey()),
> newView.getMembers().stream()).forEach(member -> ...)}}.
> Alternatives are described here:
> https://www.baeldung.com/java-combine-multiple-collections
> Once we have the combined {{Iterable}} we can use something like
> {{Collections.min()}} to find the minimum in one swell foop and this whole
> thing collapses to one or two declarative expressions.
> When this story is complete, the functionality will be in a separate method
> and we'll have a unit test for it.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
