adelapena commented on code in PR #2506:
URL: https://github.com/apache/cassandra/pull/2506#discussion_r1276149832
##########
test/unit/org/apache/cassandra/db/ReadResponseTest.java:
##########
@@ -162,12 +162,8 @@ public void makeDigestDoesntConsiderRepairedDataInfo()
}
private void verifySerDe(ReadResponse response) {
- // check that roundtripping through ReadResponse.serializer behaves as
expected.
- // ReadResponses from pre-4.0 nodes will never contain repaired data
digest
- // or pending session info, but we run all messages through both
pre/post 4.0
- // serde to check that the defaults are correctly applied
+ // check that roundtripping through ReadResponse.serializer behaves as
expected
roundTripSerialization(response, MessagingService.current_version);
Review Comment:
`roundTripSerialization` now always takes the same version argument. I'd
either inline it or annotate the method with
`@SuppressWarnings("SameParameterValue")`.
##########
test/burn/org/apache/cassandra/net/MessageGenerator.java:
##########
@@ -159,15 +158,13 @@ static Header readHeader(DataInputPlus in, int
messagingVersion) throws IOExcept
static void writeLength(byte[] payload, DataOutputPlus out, int
messagingVersion) throws IOException
{
- if (messagingVersion < VERSION_40)
- out.writeInt(payload.length);
- else
- out.writeUnsignedVInt32(payload.length);
+ assert messagingVersion >= VERSION_40;
+ out.writeUnsignedVInt32(payload.length);
}
static long serializedSize(byte[] payload, int messagingVersion)
{
- return payload.length + (messagingVersion < VERSION_40 ? 4 :
VIntCoding.computeUnsignedVIntSize(payload.length));
+ return payload.length +
VIntCoding.computeUnsignedVIntSize(payload.length);
Review Comment:
`serializedSize` doesn't use the `messagingVersion` parameter anymore, we
can remove it.
##########
src/java/org/apache/cassandra/locator/InetAddressAndPort.java:
##########
@@ -347,17 +345,10 @@ public void serialize(InetSocketAddress endpoint,
DataOutputPlus out, int versio
void serialize(byte[] address, int port, DataOutputPlus out, int
version) throws IOException
{
- if (version >= MessagingService.VERSION_40)
- {
- out.writeByte(address.length + 2);
- out.write(address);
- out.writeShort(port);
- }
- else
- {
- out.writeByte(address.length);
- out.write(address);
- }
+ assert version >= MessagingService.VERSION_40;
Review Comment:
Totally optional since it's unrelated to the changes, but we can remove the
unneeded `@SuppressWarnings("UnstableApiUsage")` class-level annotation.
##########
test/unit/org/apache/cassandra/net/ForwardingInfoTest.java:
##########
@@ -41,12 +41,6 @@ public void testCurrent() throws Exception
testVersion(MessagingService.current_version);
}
- @Test
- public void test30() throws Exception
Review Comment:
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")`.
##########
test/unit/org/apache/cassandra/locator/InetAddressAndPortSerializerTest.java:
##########
@@ -37,8 +37,10 @@ public void testRoundtrip() throws Exception
InetAddressAndPort ipv4 = InetAddressAndPort.getByName("127.0.0.1:42");
InetAddressAndPort ipv6 =
InetAddressAndPort.getByName("[2001:db8:0:0:0:ff00:42:8329]:42");
- testAddress(ipv4, MessagingService.VERSION_30);
- testAddress(ipv6, MessagingService.VERSION_30);
+ testAddress(ipv4, MessagingService.VERSION_40);
+ testAddress(ipv6, MessagingService.VERSION_40);
+ testAddress(ipv4, MessagingService.minimum_version);
+ testAddress(ipv6, MessagingService.minimum_version);
Review Comment:
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.
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -605,6 +605,7 @@ static class Serializer implements
IVersionedSerializer<CompressionParams>
{
public void serialize(CompressionParams parameters, DataOutputPlus
out, int version) throws IOException
{
+ assert version >= MessagingService.VERSION_40;
Review Comment:
It's not related to the changes but, since we are here, we can remove the
unneeded class-level `@SuppressWarnings("deprecation")` annotation. That
existed due to the deprecated properties `SSTABLE_COMPRESSION`,
`CHUNK_LENGTH_KB` and `CRC_CHECK_CHANCE`, which aren't here anymore. Feel free
to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
