[jira] [Commented] (GEODE-8298) member version comparison sense inconsistent when deciding on multicast

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-8298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17157709#comment-17157709 ] 

ASF GitHub Bot commented on GEODE-8298:
---------------------------------------

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


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