[GitHub] [geode] bschuchardt commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning

[GitBox <gi...@apache.org>]

bschuchardt commented on a change in pull request #5344:
URL: https://github.com/apache/geode/pull/5344#discussion_r450421335



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
##########
@@ -2038,7 +2039,8 @@ protected boolean chunkEntries(DistributedRegion rgn, int chunkSizeInBytes,
                     entry.key = key;
                     entry.setVersionTag(stamp.asVersionTag());
                     fillRes = mapEntry.fillInValue(rgn, entry, in, rgn.getDistributionManager(),
-                        sender.getVersionObject());
+                        Versioning

Review comment:
       I think this should be pulled out of the loop.  It's invariant, so we don't need to be doing this calculation every time we invoke fillInValue().

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
##########
@@ -2050,7 +2052,8 @@ protected boolean chunkEntries(DistributedRegion rgn, int chunkSizeInBytes,
                   entry = new InitialImageOperation.Entry();
                   entry.key = key;
                   fillRes = mapEntry.fillInValue(rgn, entry, in, rgn.getDistributionManager(),
-                      sender.getVersionObject());
+                      Versioning

Review comment:
       same here - pull it out of the loop

##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/Versioning.java
##########
@@ -26,11 +26,40 @@
 public class Versioning {
   private Versioning() {}
 
+  /**
+   * Make a VersionOrdinal for the short ordinal value.
+   *
+   * If the short ordinal represents a known version (Version) then return
+   * that instead of constructing a new VersionOrdinal.
+   *
+   * @return a known version (Version) if possible, otherwise a VersionOrdinal.
+   */
   public static VersionOrdinal getVersionOrdinal(final short ordinal) {
-    try {
-      return Version.fromOrdinal(ordinal);
-    } catch (final UnsupportedSerializationVersionException e) {
-      return new VersionOrdinalImpl(ordinal);
+    final Version knownVersion = Version.getKnownVersion(ordinal, null);
+    if (knownVersion == null) {
+      return new UnknownVersion(ordinal);
+    } else {
+      return knownVersion;
+    }
+  }
+
+  /**
+   * Return the known version (Version) for the VersionOrdinal, if possible.
+   * Otherwise return the returnWhenUnknown Version. This method essentially
+   * downcasts a {@link VersionOrdinal} to a known version {@link Version}
+   *
+   * @param anyVersion came from a call to {@link #getVersionOrdinal(short)} or this
+   *        method
+   * @param returnWhenUnknown will be returned if anyVersion does not represent
+   *        a known version
+   * @return a known version
+   */
+  public static Version getKnownVersion(final VersionOrdinal anyVersion,
+      Version returnWhenUnknown) {
+    if (anyVersion instanceof Version) {

Review comment:
       It seems like it would be more efficient to have an isKnownVersion() method than to be using instanceof.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -2077,13 +2078,13 @@ private void readGemfireVersionRecord(DataInput dis, File f) throws IOException
   }
 
   private Version readProductVersionRecord(DataInput dis, File f) throws IOException {
-    Version recoveredGFVersion;
-    short ver = Version.readOrdinal(dis);
-    try {
-      recoveredGFVersion = Version.fromOrdinal(ver);
-    } catch (UnsupportedSerializationVersionException e) {
+    short ver = VersioningIO.readOrdinal(dis);
+    final Version recoveredGFVersion =
+        Versioning.getKnownVersion(
+            Versioning.getVersionOrdinal(ver), null);

Review comment:
       You're using this pattern a lot.  Versioning.getVersionOrdinal() is going to possibly create an unknown version object and then getKnownVersion() is going to throw it away.  Is there a way to do this w/o creating an object?  Maybe Versioning.getKnownVersion(short, default)?




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