[
https://issues.apache.org/jira/browse/GEODE-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17152352#comment-17152352
]
ASF GitHub Bot commented on GEODE-8330:
---------------------------------------
Bill commented on a change in pull request #5344:
URL: https://github.com/apache/geode/pull/5344#discussion_r450497529
##########
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:
`getVersionOrdinal(short)` only constructs an `UnknownVersion` instance
if the `short` does not map to a known version (`Version`). If the `short` maps
to a known version (`Version`) then that object is returned. In that case, the
subsequent call to `getKnownVersion(VersionOrdinal,Version)` is a merely a
downcast (`instanceof` call followed by return.) **No `UnknownVersion` becomes
garbage prematurely in this scenario.**
If we introduce `getKnownVersion(short,default)` we'd save one `instanceof`
call in this case, but we'd then have two methods for converting `short` into a
version. I think there is value in the orthogonality we currently have in the
`Versioning` API: one way to convert a `short` to a `VersionOrdinal` and one
way to convert a `VersionOrdinal` into a known version (`Version`.) Two methods
in the `Versioning` API instead of three.
I feel this pattern is plenty fast (nanoseconds) relative to the number of
times we need to call these methods.
----------------------------------------------------------------
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:
[email protected]
> 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)
