You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/07/02 19:07:13 UTC
[GitHub] [geode] Bill opened a new pull request #5344: Versioning cleanup post-GEODE-8240
Bill opened a new pull request #5344:
URL: https://github.com/apache/geode/pull/5344
**not ready for review**
This is a follow-on PR to the versioning repair done in GEODE-8240. 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 releases waiting for that work. Also we didn't want to increase the complexity of the back-porting work.
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
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.) **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:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5344:
URL: https://github.com/apache/geode/pull/5344#discussion_r450505543
##########
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:
pulled ✓
##########
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:
pulled ✓
----------------------------------------------------------------
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
[GitHub] [geode] Bill edited a comment on pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill edited a comment on pull request #5344:
URL: https://github.com/apache/geode/pull/5344#issuecomment-654963614
The DistributedTestOpenJD11 failure is due to https://issues.apache.org/jira/browse/GEODE-7710
@bschuchardt to your point about `getKnownVersion()` calls not being understandable: what if the signature was changed to be more like the JDK's `Map.getOrDefault(Object key, V defaultValue)`? Something like:
```java
getKnownVersionOrDefault(final short ordinal, final Version defaultKnownVersion)
```
----------------------------------------------------------------
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
[GitHub] [geode] Bill edited a comment on pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill edited a comment on pull request #5344:
URL: https://github.com/apache/geode/pull/5344#issuecomment-654963614
The DistributedTestOpenJD11 failure is due to https://issues.apache.org/jira/browse/GEODE-7710
@bschuchardt to your point about `getKnownVersion()` calls not being understandable: what if the signature was changed to be more like the JDK's `Map.getOrDefault(Object key, V defaultValue)`? Something like:
```java
getKnownVersionOrDefault(final short ordinal, final Version defaultKnownVersion)
```
That gives rise to call sites that look like:
<img width="1081" alt="image" src="https://user-images.githubusercontent.com/4002/86813144-a65a7580-c034-11ea-9a23-77ecf56858ad.png">
<img width="771" alt="image" src="https://user-images.githubusercontent.com/4002/86813078-95116900-c034-11ea-8478-701f8fbee394.png">
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill commented on pull request #5344:
URL: https://github.com/apache/geode/pull/5344#issuecomment-654963614
The DistributedTestOpenJD11 failure is due to https://issues.apache.org/jira/browse/GEODE-7710
@bschuchardt to your point about `getKnownVersion()` calls not being understandable: what if the signature was changed to be more like the JDK's `Map.getOrDefault(Object key, V defaultValue)`? Something like:
```java
getKnownVersionOrDefault(final short ordinal, final Version 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
[GitHub] [geode] Bill merged pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill merged pull request #5344:
URL: https://github.com/apache/geode/pull/5344
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5344:
URL: https://github.com/apache/geode/pull/5344#discussion_r450504984
##########
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 might be more efficient. Or it might not be. What is certain, is that it would add a method to the `VersionOrdinal` interface.
My thinking is that if we write our deserialization code correctly then we need to do this version determination at most once when establishing a network connection (to detect the serialization version of the counterparty) or once when reading from a serialized file on disk (to detect the serialization version of the content in the file). Establishing a network connection or opening a file for reading take on the order of a millisecond whereas `instanceof` will take on the order of nanoseconds.
If you find my reasoning sound, I'd like to leave the `instanceof` call here instead of adding a predicate to `VersionOrdinal`.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] Bill commented on pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by GitBox <gi...@apache.org>.
Bill commented on pull request #5344:
URL: https://github.com/apache/geode/pull/5344#issuecomment-655070745
I changed that method as shown in the previous comment. I hope, @bschuchardt, that change makes the idiom sufficiently readable. I'm merging now since I have approval.
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5344: GEODE-8330: Structural Improvements to Versioning
Posted by 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