[Proposal] - Cleanup Protocol Version Checks
We all do lots of cleanup as we go through areas of the source, like optimizing
lambda expressions, renaming variables with more meaningful names and deleting
commented out code, just to name a few. Littered throughout the network
protocol sources are checks for older protocol versions. Lots of these checks
go back to versions of the original product Geode was granted. Some of those
versions date back a decade or more. It is undoubtably filled with obsolete
bits of unexecuted code.
I propose that we cleanup, as we go through sources for other changes, all
checks for versions older than GFE_82 AKA GFE_82_ORDINAL AKA 40. This allows
for clients just a single minor older than the first Geode incubating release
to continue to work with Geode 1.x for the purpose of uninterrupted rolling
upgrades. Upon the eventual move to Geode 2.x we would agree on some new
minimum backwards compatibility version of Geode 1.x to clean up to.
Removing this old compatibility code should remove a significant number of
unused branches of source and improve the readability of methods littered with
version checks.
Examples:
BaseCommand.java:216 (BaseCommand::shouldMasqueradeForTx):
protected boolean shouldMasqueradeForTx(Message clientMessage,
ServerConnection serverConnection) {
return serverConnection.getClientVersion().isNotOlderThan(KnownVersion.GFE_66)
&& clientMessage.getTransactionId() > TXManagerImpl.NOTX;
}
Becomes:
protected boolean shouldMasqueradeForTx(Message clientMessage) {
return clientMessage.getTransactionId() > TXManagerImpl.NOTX;
}
Even further, since this method is only used in a single place it could be
easily inlined and maintain readability.
ClientSideHandshakeImpl.java:170 (ClientSideHandshakeImpl::handshakeWithServer)
has 4 different version checks in the same method and could be reduced to 1.
To facilitate these changes I also propose that we update the handshakes to
deny connections if the protocol version is less than GFE_82. Doing so will
prevent an older client from sort of working and failing in spectacular and
undefined ways. I would also like to deprecate, with annotations, the older
version constants so that they stand out as needing to be pruned when you are
editing sources.
To reiterate this isn’t a proposal to open a PR to strip out all the old
versions in one shot. It is a proposal to remove them when submitting PRs in an
area of code that has them.
Feedback appreciated by 12/18 before a formal acceptance vote is requested.
Thanks,
Jake
Canceled: apache/geode-site#148 (master - 3b00308)
Build Update for apache/geode-site - Build: #148 Status: Canceled Duration: ? Commit: 3b00308 (master) Author: Dave Barnes Message: Update .htaccess to Geode 1.13, add geode-native doc redirects View the changeset: https://github.com/apache/geode-site/compare/4317867e3677...3b0030851963 View the full build log and details: https://travis-ci.com/github/apache/geode-site/builds/207369245?utm_medium=notification&utm_source=email Restart your build: https://travis-ci.com/github/apache/geode-site/builds/207369245?utm_medium=notification&utm_source=email -- You can unsubscribe from build emails from the apache/geode-site repository going to https://travis-ci.com/account/preferences/unsubscribe?repository=16807659&utm_medium=notification&utm_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.com/account/preferences/unsubscribe?utm_medium=notification&utm_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Canceled: apache/geode-site#149 (master - 98bf623)
Build Update for apache/geode-site - Build: #149 Status: Canceled Duration: ? Commit: 98bf623 (master) Author: Dave Barnes Message: Modify geode-native api doc redirects View the changeset: https://github.com/apache/geode-site/compare/3b0030851963...98bf623d59a7 View the full build log and details: https://travis-ci.com/github/apache/geode-site/builds/207370961?utm_medium=notification&utm_source=email Restart your build: https://travis-ci.com/github/apache/geode-site/builds/207370961?utm_medium=notification&utm_source=email -- You can unsubscribe from build emails from the apache/geode-site repository going to https://travis-ci.com/account/preferences/unsubscribe?repository=16807659&utm_medium=notification&utm_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.com/account/preferences/unsubscribe?utm_medium=notification&utm_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Passed: apache/geode-site#150 (master - ce9e3f8)
Build Update for apache/geode-site - Build: #150 Status: Passed Duration: 41 secs Commit: ce9e3f8 (master) Author: Dave Barnes Message: Update README file with docs redirect info View the changeset: https://github.com/apache/geode-site/compare/98bf623d59a7...ce9e3f8fd819 View the full build log and details: https://travis-ci.com/github/apache/geode-site/builds/207376894?utm_medium=notification&utm_source=email -- You can unsubscribe from build emails from the apache/geode-site repository going to https://travis-ci.com/account/preferences/unsubscribe?repository=16807659&utm_medium=notification&utm_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.com/account/preferences/unsubscribe?utm_medium=notification&utm_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
