You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Jacob Barrett <ja...@vmware.com> on 2021/01/20 18:08:04 UTC

Re: [Proposal] - Cleanup Protocol Version Checks

Given the lack of objections to this proposal I have moved forward with some of this proposal. In deeper investigation it has come to light that GFE_82 was never used and appeared only as an artifact of open sourcing preparations. I have therefore retargeted GFE_81 as the oldest ordinal version supported by Geode. This value covers the last proprietary release prior to Geode’s open sourcing.

https://issues.apache.org/jira/browse/GEODE-8837

On Dec 8, 2020, at 2:38 PM, Jacob Barrett <ja...@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

Re: [Proposal] - Cleanup Protocol Version Checks

Posted by Jacob Barrett <ja...@vmware.com>.


On Jan 20, 2021, at 10:36 AM, Owen Nichols <on...@vmware.com>> wrote:

Sounds like good cleanup, but I'm not sure I understand why you propose breaking up the work and sneaking it in through mostly-unrelated PRs?  My first thought would be do it in one shot, start by deleting the old constants, and let the compiler guide you to all code that needs pruning.  Or, if for some reason it needs to be done/tested iteratively, a feature branch or a long-lived PR might be an appropriate way to segment the work.

Just like we don’t cleanup all our code in one giant shot, there is too much to tackle at one time for one person to bite it off. This makes large scale cleanup efforts fail to take off. If however, you are in a chunk of code and the compiler and static analyzers are complaining that the file has things to clean up then we should all be compelled to leave it better than we found it. This proposal lays the ground work to add version checking into this large set of cleanup that needs to happen. While it isn’t ideal for some cleanup to linger it is certainly better to do part now than none at all.

-Jake

Re: [Proposal] - Cleanup Protocol Version Checks

Posted by Owen Nichols <on...@vmware.com>.

Sounds like good cleanup, but I'm not sure I understand why you propose breaking up the work and sneaking it in through mostly-unrelated PRs? My first thought would be do it in one shot, start by deleting the old constants, and let the compiler guide you to all code that needs pruning. Or, if for some reason it needs to be done/tested iteratively, a feature branch or a long-lived PR might be an appropriate way to segment the work.

On 1/20/21, 10:08 AM, "Jacob Barrett" <ja...@vmware.com> wrote:

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FGEODE-8837&amp;data=04%7C01%7Conichols%40vmware.com%7Cbe702eae5ed14f116f0208d8bd6e5d81%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637467629038062309%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eVg1ktB5TLoTJd5n3o05XyLFE84DoELbvj9Do4xv9u4%3D&amp;reserved=0

On Dec 8, 2020, at 2:38 PM, Jacob Barrett <ja...@vmware.com>> wrote:

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):

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 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