+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