[GEODE-8298] member version comparison sense inconsistent when deciding on multicast

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.14.0
• Component/s: membership
• Labels: pull-request-available starter
• Assignee: Kamilla Aslami
• Reporter: Bill Burcham
• Created: 25/Jun/20 16:51
• Updated: 03/Sep/21 02:23
• Resolved: 20/Jul/20 16:50

Description

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:

    // 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);

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.

Issue Links

• links to: GitHub Pull Request #5370