adelapena commented on code in PR #2506:
URL: https://github.com/apache/cassandra/pull/2506#discussion_r1272167052
##########
src/java/org/apache/cassandra/hints/HintsDescriptor.java:
##########
@@ -231,8 +231,6 @@ static int messagingVersion(int hintsVersion)
{
switch (hintsVersion)
{
- case VERSION_30:
- return MessagingService.VERSION_30;
Review Comment:
This leaves `HintsDescriptor.VERSION_30` unused, it can probably be removed.
##########
src/java/org/apache/cassandra/net/Message.java:
##########
@@ -711,15 +708,12 @@ private Serializer()
public <T> void serialize(Message<T> message, DataOutputPlus out, int
version) throws IOException
{
- if (version >= VERSION_40)
- serializePost40(message, out, version);
- else
- serializePre40(message, out, version);
+ serializePost40(message, out, version);
Review Comment:
All the methods that were named `*Post40` can be renamed or inlined into the
called methods, since they don't have a `*Pre40` counterpart anymore.
##########
src/java/org/apache/cassandra/db/filter/ColumnFilter.java:
##########
@@ -1004,8 +1003,6 @@ private void
serializeRegularAndStaticColumns(RegularAndStaticColumns regularAnd
public ColumnFilter deserialize(DataInputPlus in, int version,
TableMetadata metadata) throws IOException
{
int header = in.readUnsignedByte();
- // The meaning of isFetchAll is actually different for pre-4.0
versions and for 4.0+ versions
- // In 4.0+ it meant is fetch all regulars
Review Comment:
There is a similar obsolete comment on `FETCH_ALL_MASK`. We could also
rename both `isFetchAll` and `FETCH_ALL_MASK` to `isFetchAllRegulars` and
`FETCH_ALL_REGULARS_MASK`.
##########
src/java/org/apache/cassandra/net/ParamType.java:
##########
@@ -46,19 +45,9 @@ public enum ParamType
FORWARD_TO (0, "FWD_TO",
ForwardingInfo.serializer),
RESPOND_TO (1, "FWD_FRM", fwdFrmSerializer),
- @Deprecated
- FAILURE_RESPONSE (2, "FAIL", LegacyFlag.serializer),
Review Comment:
Maybe we should add a comment on `ParamType.id` saying that the ids are nor
continuous because some deprecated param types have been removed? Or maybe left
a comment for every removed param type?
##########
src/java/org/apache/cassandra/net/OutboundConnectionInitiator.java:
##########
@@ -350,68 +349,35 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out)
FrameEncoder frameEncoder = null;
Result<SuccessType> result;
- if (useMessagingVersion > 0)
+ assert useMessagingVersion > 0;
+
+ if (useMessagingVersion < settings.acceptVersions.min ||
useMessagingVersion > settings.acceptVersions.max)
{
- if (useMessagingVersion < settings.acceptVersions.min ||
useMessagingVersion > settings.acceptVersions.max)
- {
- result = incompatible(useMessagingVersion,
peerMessagingVersion);
- }
- else
- {
- // This is a bit ugly
- if (type.isMessaging())
- {
- switch (settings.framing)
- {
- case LZ4:
- frameEncoder =
FrameEncoderLZ4.fastInstance;
- break;
- case CRC:
- frameEncoder = FrameEncoderCrc.instance;
- break;
- case UNPROTECTED:
- frameEncoder =
FrameEncoderUnprotected.instance;
- break;
- }
-
- result = (Result<SuccessType>)
messagingSuccess(ctx.channel(), useMessagingVersion, frameEncoder.allocator());
- }
- else
- {
- result = (Result<SuccessType>)
streamingSuccess(ctx.channel(), useMessagingVersion);
- }
- }
+ result = incompatible(useMessagingVersion,
peerMessagingVersion);
}
else
{
- assert type.isMessaging();
-
- // pre40 handshake responses only (can be a post40 node)
- if (peerMessagingVersion == requestMessagingVersion
- || peerMessagingVersion > settings.acceptVersions.max)
// this clause is for impersonating 3.0 node in testing only
+ // This is a bit ugly
+ if (type.isMessaging())
{
switch (settings.framing)
{
+ case LZ4:
+ frameEncoder = FrameEncoderLZ4.fastInstance;
+ break;
case CRC:
- case UNPROTECTED:
- frameEncoder = FrameEncoderLegacy.instance;
+ frameEncoder = FrameEncoderCrc.instance;
break;
- case LZ4:
- frameEncoder = FrameEncoderLegacyLZ4.instance;
+ case UNPROTECTED:
+ frameEncoder =
FrameEncoderUnprotected.instance;
break;
}
- result = (Result<SuccessType>)
messagingSuccess(ctx.channel(), requestMessagingVersion,
frameEncoder.allocator());
+ result = (Result<SuccessType>)
messagingSuccess(ctx.channel(), useMessagingVersion, frameEncoder.allocator());
Review Comment:
Nit: we can add `@SuppressWarnings("unchecked")`
##########
src/java/org/apache/cassandra/service/pager/PagingState.java:
##########
@@ -402,7 +402,7 @@ public static RowMark create(TableMetadata metadata, Row
row, ProtocolVersion pr
{
// We froze the serialization version to 3.0 as we need to
make this this doesn't change (that is, it has to be
// fix for a given version of the protocol).
- mark = Clustering.serializer.serialize(row.clustering(),
MessagingService.VERSION_30, makeClusteringTypes(metadata));
+ mark = Clustering.serializer.serialize(row.clustering(),
MessagingService.VERSION_40, makeClusteringTypes(metadata));
Review Comment:
The comment right above seems to need an update.
--
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]
