Lift MessagingService.minimum_version to 40 (CASSANDRA-18314)

[michaelsembwever]

No description provided.

[michaelsembwever]

all comments addressed.
rebased off trunk.
CI runs: https://app.circleci.com/pipelines/github/michaelsembwever/cassandra/201

[adelapena]

I think another thing that can be slightly simplified now that we don't expect 3.0 nodes is QueryProcessor#useNewPreparedStatementBehaviour(), where we don't need QueryProcessor.NEW_PREPARED_STATEMENT_BEHAVIOUR_SINCE_30 nor QueryProcessor.NEW_PREPARED_STATEMENT_BEHAVIOUR_SINCE_3X.

The JavaDoc for QueryProcessor#prepare should also be updated, since it contains some references to upgrades from 3.0.26 and 3.11.

[michaelsembwever]

I think another thing that can be slightly simplified now that we don't expect 3.0 nodes is QueryProcessor

can we do that in a separate ticket, not related to MessagingService (see parent ticket)

[adelapena]

I think another thing that can be slightly simplified now that we don't expect 3.0 nodes is QueryProcessor

can we do that in a separate ticket, not related to MessagingService (see parent ticket)

Yeah, it can be a separate ticket since the min cluster version doesn't come from MessagingService.

Edit: Created CASSANDRA-18695 for not considering 3.x nodes in QueryProcessor#useNewPreparedStatementBehaviour

[Mmuzaf]

These methods are not used and I think can be removed:

• FwdFrmSerializer#pre40DeserializeWithLength
• Verb#toPre40Verb

Do we need to generate benchmarks for a 'pre40'?
MessageOutBench#serializePre40

[Mmuzaf]

            // if the other node is pre-4.0, it will respond only with its maxMessagingVersion
            if (maxMessagingVersion < VERSION_40 || handshakeMessagingVersion < VERSION_40)
                return new Accept(useMessagingVersion, maxMessagingVersion);        

Should we return null for this case? This is the HandshakeProtocol class.

[michaelsembwever]

This is the HandshakeProtocol class.

changing that breaks HandshakeTest 🤷

[adelapena]

 // if the other node is pre-4.0, it will respond only with its maxMessagingVersion
if (maxMessagingVersion < VERSION_40 || handshakeMessagingVersion < VERSION_40)
   return new Accept(useMessagingVersion, maxMessagingVersion);        

Should we return null for this case? This is the HandshakeProtocol class.

changing that breaks HandshakeTest 🤷

Returning null there doesn't break HandshakeTest for me, at least locally. What failure are you finding?

[michaelsembwever]

Configuration: EncryptionOptions.ServerEncryptionOptions#legacy_ssl_storage_port_enabled

this is related to something else than the messaging service. a separate ticket under the epic CASSANDRA-18306 is warranted.

[Mmuzaf]

I have found a few if-else statements if (version >= MessagingService.VERSION_40) where the else part can be safely removed as this condition is always true from now on, right?

The production code classes:

• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/ReadResponse.java#L343
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/locator/InetAddressAndPort.java#L356
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/locator/InetAddressAndPort.java#L431
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/locator/InetAddressAndPort.java#L459
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/locator/InetAddressAndPort.java#L475
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/locator/InetAddressAndPort.java#L506
• https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/schema/CompressionParams.java#L618

[Mmuzaf]

The ForwardingInfo class:

• all of the if-else statements version >= VERSION_40 can be simplified, right?
• according to the TODO statement 'in the next protocol version only serialize peers, message id will become redundant once 3.0 is out of the picture' we can get rid of messageIds field or am I missing something?

[michaelsembwever]

all comments addressed.

[Mmuzaf]

Sorry, the last one :-)

The RequestFailureReason class - the condition version < VERSION_40 can be simplified as well.

[Mmuzaf]

DropCompactStorage#validateCanDropCompactStorage

I think this can be removed as it is always false.

                if (MessagingService.instance().versions.knows(node) &&
                    MessagingService.instance().versions.getRaw(node) < MessagingService.VERSION_40)
                {
                    before4.add(node);
                    continue;
                }

[Mmuzaf]

ReadResponseTest#roundTripSerialization

Can remove the following:

            if (version < MessagingService.VERSION_40)
            {
                assertFalse(deser.mayIncludeRepairedDigest());
                // even though that means they should never be used, verify that the default values are present
                assertEquals(ByteBufferUtil.EMPTY_BYTE_BUFFER, deser.repairedDataDigest());
                assertTrue(deser.isRepairedDigestConclusive());
            }

[Mmuzaf]

For the Verifier class we can simplify the methods:

• willProcessOnEventLoop
• expiresAtNanos

[Mmuzaf]

The MessageGenerator class the methods can be simplified as well:

• writeLength
• readHeader

[michaelsembwever]

DropCompactStorage#validateCanDropCompactStorage

this is its own kettle of fish, separate ticket please.

[michaelsembwever]

comments addressed.

[Mmuzaf]

@michaelsembwever Thank you, re-checked the comments and sources - OK
The build with all checks passes locally.

[adelapena]

This will test VERSION_40 three times, since it's the value of MessagingService.minimum_version and MessagingService.current_version. However, VERSION_50 won't be tested. I think we could modify the test to simply use VERSION_40 and VERSION_50, and maybe make sure that minimum_version and current_version are among them.

[michaelsembwever]

i can see value in testing both explicit values and the minimum and current defined.

cost is irrelevant here.

so… i'd rather add VERSION_50 and not remove any. wdyt?

[michaelsembwever]

actually… i should loop through supportedVersions()

[adelapena]

Yes, supportedVersions() is perfect for this.

[adelapena]

Now ForwardingInfoTest#testVersion(int) is always called with the same value. I think it can be either inlined into ForwardingInfoTest#testCurrent() or annotated with @SuppressWarnings("SameParameterValue").

[michaelsembwever]

going to loop through supportedVersions()

[adelapena]

roundTripSerialization now always takes the same version argument. I'd either inline it or annotate the method with @SuppressWarnings("SameParameterValue").

[michaelsembwever]

going to loop through supportedVersions()

[michaelsembwever]

@adelapena comments addressed

[adelapena]

Last changes look good to me, +1. Do we have a CI run for them?

[michaelsembwever]

CI

• https://app.circleci.com/pipelines/github/michaelsembwever/cassandra/214

SSL broken tests are from jdk17.
upgrade timestamp tests are from the deletion serialisation merge.
SSTablesSystemViewTest is from the SAI merge.

[michaelsembwever]

latest CI

• https://app.circleci.com/pipelines/github/michaelsembwever/cassandra/215

all failures are not related.

[adelapena]

CI results look good to me. There are failures for SSTablesSystemViewTest#testVirtualTableThroughIndexLifeCycle with tries that aren't reported, but they can be reproduced on trunk:

• https://app.circleci.com/pipelines/github/adelapena/cassandra/3068/workflows/17760f03-9807-41cc-9cb3-a1fdf3e60f77/jobs/64069/tests

I have created CASSANDRA-18703 for that test failure.

As for the j17 workflow, I've started a few tests here: https://app.circleci.com/pipelines/github/adelapena/cassandra/3069/workflows/613644ed-b9d6-4d81-9ef3-ee9d13625fd5

[michaelsembwever]

Committed 4bbfd64