+1 Good suggestion.
I have no idea what the scope of the cleanup looks like, but it seems like an
excellent entry-point for newcomers to the codebase to get their hands dirty.
I know this is a weird request, but I'd like to ask committers not to jump on
this, but allow some time for non-committers to pick this up and get some PRs
under their belts.
Thanks
--Jens
On 12/8/20, 2:38 PM, "Jacob Barrett" <jabarr...@vmware.com> wrote:
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
