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 like `instanceof` 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]
