You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@geode.apache.org by "Bill Burcham (Jira)" <ji...@apache.org> on 2020/07/09 16:49:00 UTC
[jira] [Updated] (GEODE-8298) member version comparison sense
inconsistent when deciding on multicast
[ https://issues.apache.org/jira/browse/GEODE-8298?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bill Burcham updated GEODE-8298:
--------------------------------
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:
{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.
was:
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 a {{Iterable}} combinator like Guava's {{Iterables.concat()}} or Apache Commons Collections {{IterableUtils.chainedIterable()}} to combine the two iterables: {{surpriseMembers}} and {{newView.getMembers()}} into one iterable. If we don't want to introduce either of those two libraries into the product we can write our own {{concat()}} utility like the one 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.
> 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
> 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)