[GitHub] [geode] Bill commented on a change in pull request #5269: GEODE-8240: View has old locator version number after rolling upgrade

2020-06-25 Thread GitBox 


Bill commented on a change in pull request #5269:
URL: https://github.com/apache/geode/pull/5269#discussion_r442241602



##
File path: 
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
##
@@ -220,12 +220,16 @@ public InetAddress getInetAddress() {
   @Override
 
   public short getVersionOrdinal() {
-return this.versionObj.ordinal();
+return versionOrdinal;
   }
 
   @Override
   public Version getVersion() {
-return versionObj;
+try {
+  return Version.fromOrdinal(versionOrdinal);
+} catch (final UnsupportedSerializationVersionException e) {

Review comment:
   **This right here** is a big fat problem. We are silently throwing away 
information when the version ordinal represents an unknown version. One 
approach is to let the exception escape (as a checked exception.) But that 
entails fixing the 53 callers of this method. An alternative would be to 
"tunnel" the checked exception through an unchecked one. Lastly we could leave 
it as-is and try to go around and analyze whether or not the places calling 
this can survive this loss of information. That would be bad enough but what's 
worse is that we'd be introducing a maintenance trap.





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

[GitHub] [geode] Bill commented on a change in pull request #5269: GEODE-8240: View has old locator version number after rolling upgrade

2020-06-18 Thread GitBox 


Bill commented on a change in pull request #5269:
URL: https://github.com/apache/geode/pull/5269#discussion_r442241602



##
File path: 
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
##
@@ -220,12 +220,16 @@ public InetAddress getInetAddress() {
   @Override
 
   public short getVersionOrdinal() {
-return this.versionObj.ordinal();
+return versionOrdinal;
   }
 
   @Override
   public Version getVersion() {
-return versionObj;
+try {
+  return Version.fromOrdinal(versionOrdinal);
+} catch (final UnsupportedSerializationVersionException e) {

Review comment:
   **This right here** is a big fat problem. We are silently throwing away 
information when the version ordinal represents an unknown version. One 
approach is to let the exception escape (as a checked exception.) But that 
entails fixing the 53 callers of this method. An alternative would be to 
"tunnel" the checked exception through an unchecked one.





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