[
https://issues.apache.org/jira/browse/GEODE-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17152290#comment-17152290
]
ASF GitHub Bot commented on GEODE-8330:
---------------------------------------
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:
us...@infra.apache.org
> Structural Improvements to Serialization Versioning
> ---------------------------------------------------
>
> Key: GEODE-8330
> URL: https://issues.apache.org/jira/browse/GEODE-8330
> Project: Geode
> Issue Type: Improvement
> Components: serialization
> Reporter: Bill Burcham
> Assignee: Bill Burcham
> Priority: Major
> Labels: membership
>
> As a follow-on to GEODE-8240 we want to do another ticket to improve, outside
> the context of a big release-blocking bug, the versioning framework. The
> starting point for this work will be the work completed and merged for
> GEODE-8240.
> Our goals are to:
> * Make the version type hierarchy easily understood by Geode developers
> * Make the framework foolproof so we prevent bugs like GEODE-8240
> We’ll rely on a handful of techniques to accomplish this:
> * Clear naming
> * Orthogonality—separation of concerns, one way to do each thing
> * No {{null}}—it’s not needed in this framework at all
> * No exceptions (no throws)—prefer total functions instead
> * Separate “model” code from I/O code
> When fixing the rolling upgrade bug in GEODE-8240 we introduced a base type:
> {{VersionOrdinal}} for the existing "enum" {{Version}}. During the work for
> that ticket we saw lots of little things that needed cleaning up, but we
> didn't want to do them in that other PR because we had a couple support
> branches waiting for that ticket to complete. Also we wanted the GEODE-8240
> changes to be minimal so as to minimize the complexity of our back-porting
> work.
> Now that that ticket is complete and back-ported, this ticket will:
> * get rid of confusing and wrong inheritance relationship between
> {{VersionOrdinalImpl}} and {{Version}}: now {{Version}} (i.e. known version)
> and {{UnknownVersion}} both extend {{AbstractVersion}} which implements
> {{VersionOrdinal}}—improved naming of this hierarchy will come, probably in a
> follow-on PR since it would touch lots of files
> * flesh-out {{Versioning}} factory:
> ** add {{Version getKnownVersion(final VersionOrdinal anyVersion, Version
> returnWhenUnknown)}} method that simply downcasts {{anyVersion}} if it can
> ** ensure there's exactly _one way_ to construct a version
> ({{VersionOrdinal}}) from a {{short}}, and there is exactly _one way_ to get
> a known version ({{Version}}) from a {{VersionOrdinal}}
> * version acquisition no longer throws exceptions ever
> * eliminate {{InternalDistributedMember.getVersionObject()}} in favor of
> {{Versioning.getKnownVersion(Versioning.getVersionOrdinal(ver),
> Version.CURRENT)}}: the latter makes it clear to maintainers that
> {{Version.CURRENT}} will be used as a stand-in for unknown versions
> * move I/O logic to a separate class, {{VersioningIO}}
> * eliminate tons of redundant and unused methods in {{Version}}
> The type hierarchy after this story is complete will be:
> {noformat}
> VersionOrdinal
> <<interface>>
> ^
> |
> AbstractVersion
> <<abstract>>
> ^
> |
> -----------------
> | |
> Version UnknownVersion
> <<enum>>
> {noformat}
> In a separate ticket/story we will tackle the final improvements to the type
> names:
> {{Version}} will become {{KnownVersion}}
> {{VersionOrdinal}} will become {{Version}} yippee!!
> Changing {{Version}} to {{KnownVersion}} will touch hundreds of files. We'll
> tackle that in a separate ticket/story/PR that is focused just on that
> renaming.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
