[
https://issues.apache.org/jira/browse/CASSANDRA-15299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17148831#comment-17148831
]
Sam Tunnicliffe edited comment on CASSANDRA-15299 at 6/30/20, 5:08 PM:
-----------------------------------------------------------------------
Thanks for the comments [~ifesdjeen] & [~omichallat], I've pushed a few commits
to the [branch|https://github.com/beobal/cassandra/commits/15299-trunk].
{quote}
There are several things that I wanted to bring to your attention:
{quote}
I've handled most of these in a refactor of Flusher. As you suggested, for
framed items we now collate the frames and only allocate the payloads when we
flush to the netty channel. So now, we allocate the payload based on the actual
number of bytes required for the specific channel.
{quote}{{ExceptionHandlers$PostV5ExceptionHandler#exceptionCaught}}: when
flushing an exception, we don't call release on the payload.
{quote}
Included in the {{minor cleanups}} commit
{quote}There are several places in {{SimpleClient}} where
{{largePayload#release}} isn't called.
{quote}
I've refactored the flushing large messages in {{SimpleClient}} to match
{{Flusher}}, so this is working properly now.
{quote}Other things...
{quote}
{quote}{{Dispatcher#processRequest}}, we don't need to cast error to
{{Message.Response}} if we change its type to {{ErrorMessage}}.
{quote}
In {{CqlMessageHandler#releaseAfterFlush}}, we can call
{{sourceFrame#release()}} instead of {{sourceFrame.body.release()}} for
consistency with other calls
Both in {{minor cleanups}}
{quote}{{Server#requestPayloadInFlightPerEndpoint}} can be a non-static
{{Server}} member.
{quote}
If you don't mind I'd prefer to leave this as it is for now as it's
pre-existing and changing would require reworking CASSANDRA-15519 (changing
limits at runtime).
{quote}Should we hide {{flusher.queued.add()}} behind a method to disallow
accessing queue directly?
{quote}
I've done this, but I'm not 100% convinced of its utility. As the two
{{Flusher}} subclasses need access to the queue, we have to provide package
private methods {{poll}} and {{isEmpty}} as well as one to {{enqueue}}. So
unless we move {{Flusher}} to its own subpackage, the queue is effectively
visible to everything else in {{o.a.c.Transport}}
{quote}We can change the code a bit to make {{FlushItemConverter}} instances
explicit. Right now, we basically have two converters both called
{{#toFlushItem}} in {{CQLMessageHandler}} and {{LegacyDispatchHandler}}. We
could have them as inner classes. It's somewhat useful since if you change the
signature of this method, or stop using it, it'll be hard to find that it is
actually an implementation of converter.
{quote}
I've left this as it is just for the moment. I'm working on some tests which
supply a lambda to act as the converter, so I'll come back to this when those
have solidified a bit more.
{quote}Looks like {{MessageConsumer}} could be generic, since we cast it to
either request or response.
{quote}
I've parameterised {{MessageConsumer}} & {{CQLMessageHandler}} according to the
subclass of Message they expect and extended this a bit by moving the logic out
of {{Message$ProtocolEncoder}} to an abstract {{Message$Decoder<M extends
Message>}} with concrete subclasses for {{Request}} and {{Response}}.
{quote}Looks like {{CQLMessageHandler#processCorruptFrame}}, initially had an
intention of handling recovery, but now just throws a CRC exception regardless.
This does match description, but usage of {{isRecoverable}} seems to be
redundant here, unless we change semantics of recovery.
{quote}
It is somewhat redundant here, except that it logs a slightly different message
to indicate whether the CRC mismatch was found in the frame header or body.
I'll leave it as it is for now as it's technically possible to recover from a
corrupt body, but would be problematic for clients just now.
I still have some comments to address, as well as those from [~omichallat] ...
{quote}{{Frame$Decoder}} and other classes that are related to legacy path can
be extracted to a separate class, since {{Frame}} itself is still useful, but
classes that facilitate legacy encoding/decoding/etc can be extracted.
{quote}
{quote}{{Frame#encodeHeaderInto}} seems to be duplicating the logic we have in
{{Frame$Encoder#encodeHeader}}, should we unify the two? Maybe we can have
encoding/decoding methods shared for both legacy and new paths, for example, as
static methods?
{quote}
{quote}As you have mentioned, it would be great to rename {{Frame}} to
something different, like {{Envelope}}, since right now we have
{{FrameDecoder#Frame}} and {{Frame$Decoder}} and variable names that correspond
with class names, which makes it all hard to follow.
{quote}
was (Author: beobal):
Thanks for the comments [~ifesdjeen] & [~omichallat], I've pushed a few commits
to the [branch|https://github.com/beobal/cassandra/commits/15299-trunk].
{quote}
There are several things that I wanted to bring to your attention:
{quote}
I've handled most of these in a refactor of Flusher. As you suggested, for
framed items we now collate the frames and only allocate the payloads when we
flush to the netty channel. So now, we allocate the payload based on the actual
number of bytes required for the specific channel.
{quote}{{ExceptionHandlers$PostV5ExceptionHandler#exceptionCaught}}: when
flushing an exception, we don't call release on the payload.
{quote}
Included in the {{minor cleanups}} commit
{quote}There are several places in {{SimpleClient}} where
{{largePayload#release}} isn't called.
{quote}
I've refactored the flushing large messages in {{SimpleClient}} to match
{{Flusher}}, so this is working properly now.
{quote}Other things...
{quote}
{quote}{{Dispatcher#processRequest}}, we don't need to cast error to
{{Message.Response}} if we change its type to {{ErrorMessage}}.
{quote}
In {{CqlMessageHandler#releaseAfterFlush}}, we can call
{{sourceFrame#release()}} instead of {{sourceFrame.body.release()}} for
consistency with other calls
Both in {{minor cleanups}}
{quote}{{Server#requestPayloadInFlightPerEndpoint}} can be a non-static
{{Server}} member.
{quote}
If you don't mind I'd prefer to leave this as it is for now as it's
pre-existing and changing would require reworking CASSANDRA-15519 (changing
limits at runtime).
{quote}Should we hide {{flusher.queued.add()}} behind a method to disallow
accessing queue directly?
{quote}
I've done this, but I'm not 100% convinced of its utility. As the two
{{Flusher}} subclasses need access to the queue, we have to provide package
private methods {{poll}} and {{isEmpty}} as well as one to {{enqueue}}. So
unless we move {{Flusher}} to its own subpackage, the queue is effectively
visible to everything else in {{o.a.c.Transport}}
{quote}We can change the code a bit to make {{FlushItemConverter}} instances
explicit. Right now, we basically have two converters both called
{{#toFlushItem}} in {{CQLMessageHandler}} and {{LegacyDispatchHandler}}. We
could have them as inner classes. It's somewhat useful since if you change the
signature of this method, or stop using it, it'll be hard to find that it is
actually an implementation of converter.
{quote}
> I've left this as it is just for the moment. I'm working on some tests which
> supply a lambda to act as the converter, so I'll come back to this when those
> have solidified a bit more.
{quote}Looks like {{MessageConsumer}} could be generic, since we cast it to
either request or response.
{quote}
I've parameterised {{MessageConsumer}} & {{CQLMessageHandler}} according to the
subclass of Message they expect and extended this a bit by moving the logic out
of {{Message$ProtocolEncoder}} to an abstract\{{ Message$Decoder<M extends
Message>}} with concrete subclasses for {{Request}} and {{Response}}.
>bq.Looks like {{CQLMessageHandler#processCorruptFrame}}, initially had an
>intention of handling recovery, but now just throws a CRC exception
>regardless. This does match description, but usage of {{isRecoverable}} seems
>to be redundant here, unless we change semantics of recovery.
It is somewhat redundant here, except that it logs a slightly different message
to indicate whether the CRC mismatch was found in the frame header or body.
I'll leave it as it is for now as it's technically possible to recover from a
corrupt body, but would be problematic for clients just now.
I still have some comments to address, as well as those from [~omichallat] ...
{quote}Frame$Decoder and other classes that are related to legacy path can be
extracted to a separate class, since Frame itself is still useful, but classes
that facilitate legacy encoding/decoding/etc can be extracted.
{quote}
{quote}Frame#encodeHeaderInto seems to be duplicating the logic we have in
Frame$Encoder#encodeHeader, should we unify the two? Maybe we can have
encoding/decoding methods shared for both legacy and new paths, for example, as
static methods?
{quote}
{quote}As you have mentioned, it would be great to rename Frame to something
different, like Envelope, since right now we have FrameDecoder#Frame and
Frame$Decoder and variable names that correspond with class names, which makes
it all hard to follow.
{quote}
> CASSANDRA-13304 follow-up: improve checksumming and compression in protocol
> v5-beta
> -----------------------------------------------------------------------------------
>
> Key: CASSANDRA-15299
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15299
> Project: Cassandra
> Issue Type: Improvement
> Components: Messaging/Client
> Reporter: Aleksey Yeschenko
> Assignee: Sam Tunnicliffe
> Priority: Normal
> Labels: protocolv5
> Fix For: 4.0-alpha
>
>
> CASSANDRA-13304 made an important improvement to our native protocol: it
> introduced checksumming/CRC32 to request and response bodies. It’s an
> important step forward, but it doesn’t cover the entire stream. In
> particular, the message header is not covered by a checksum or a crc, which
> poses a correctness issue if, for example, {{streamId}} gets corrupted.
> Additionally, we aren’t quite using CRC32 correctly, in two ways:
> 1. We are calculating the CRC32 of the *decompressed* value instead of
> computing the CRC32 on the bytes written on the wire - losing the properties
> of the CRC32. In some cases, due to this sequencing, attempting to decompress
> a corrupt stream can cause a segfault by LZ4.
> 2. When using CRC32, the CRC32 value is written in the incorrect byte order,
> also losing some of the protections.
> See https://users.ece.cmu.edu/~koopman/pubs/KoopmanCRCWebinar9May2012.pdf for
> explanation for the two points above.
> Separately, there are some long-standing issues with the protocol - since
> *way* before CASSANDRA-13304. Importantly, both checksumming and compression
> operate on individual message bodies rather than frames of multiple complete
> messages. In reality, this has several important additional downsides. To
> name a couple:
> # For compression, we are getting poor compression ratios for smaller
> messages - when operating on tiny sequences of bytes. In reality, for most
> small requests and responses we are discarding the compressed value as it’d
> be smaller than the uncompressed one - incurring both redundant allocations
> and compressions.
> # For checksumming and CRC32 we pay a high overhead price for small messages.
> 4 bytes extra is *a lot* for an empty write response, for example.
> To address the correctness issue of {{streamId}} not being covered by the
> checksum/CRC32 and the inefficiency in compression and checksumming/CRC32, we
> should switch to a framing protocol with multiple messages in a single frame.
> I suggest we reuse the framing protocol recently implemented for internode
> messaging in CASSANDRA-15066 to the extent that its logic can be borrowed,
> and that we do it before native protocol v5 graduates from beta. See
> https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/FrameDecoderCrc.java
> and
> https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/FrameDecoderLZ4.java.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
