[
https://issues.apache.org/jira/browse/CASSANDRA-15299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17211482#comment-17211482
]
Caleb Rackliffe commented on CASSANDRA-15299:
---------------------------------------------
[~samt] Here are the detailed/tactical points from my first pass at review.
Some of these items, especially the testing bits, are things I could
potentially branch and work on a bit myself, so let me know if you'd like a
hand. I still want to make one more top-down pass at things Monday now that the
smaller details are clearer.
*Correctness*
- {{AbstractMessageHandler}}
-- Are we incrementing {{receivedCount}} both on the first frame of a large
message (in {{processFirstFrameOfLargeMessage()}}) and on the last frame (in
{{processSubsequentFrameOfLargeMessage()}})?
-- {{forceOverAllocation()}} only allocates enough to hit the limit, but is it
possible for the subsequent release to de-allocate the full frame size?
- {{Flusher}} uses a {{PayloadAllocator}} from either the CRC or LZ4 encoder.
CRC includes the header and trailer size when it hits the {{BufferPool}}, while
LZ4 uses the {{Payload}} constructor that doesn't take those into account. Is
this correct?
- {{PostV5ExceptionHandler}} - in {{exceptionCaught()}}, does
{{payload.release()}} need to be in a {{finally}} block?
- {{CQLMessageHandler}} - should {{processOneContainedMessage()}} release the
frame in a {{finally}} block?
- It isn't really a correctness issue per-se, but {{FrameSet}} would feel safer
if we used composition rather than inheritance. (Was the motivator not creating
the additional object?)
*Testing*
- {{CQLTester}} - It might be a little awkward to follow the semantics of
"require" -> "prepare" -> "initialize". Maybe we should just inline
{{prepareNetwork()}}.
- The large frame accumulation state machine might be a good unit test target.
Lowest level might be a test-only subclass of
{{AbstractMessageHandler.LargeMessage}} that simulates the cases we're
interested in. Another would be to go one level up and test the
{{CQLMessageHandler.LargeMessage}, but that pulls all of {{CQLMessageHandler}}
in, as it's not a static class. (We could make it static, create a little
interface to abstract {{CQLMessageHandler}} away, etc.)
- It might be worth pulling up {{WaitQueue}} (which is already static) as a
top-level class, then throw a few unit-level tests around it it.
- {{StartupMessage}} - There's enough logic that it might be nice to unit test
execute(). (Mocking {{Connection}} and checking replays, etc.)
*Documentation*
- Should describe {{native_transport_receive_queue_capacity_in_bytes}} in
{{cassandra.yaml}} and mention it in the Jira docs section?
- Is the plan to just merge the {{ql_protocol_V5_framing.asc}} content into
{{native_protocol_v5.spec}} when things solidify?
- {{cql_protocol_V5_framing.asc}} - worth mentioning somewhere there (or if
this ends up in the official V5 spec) that non-self-contained frames still only
hold part of one message and can't contain the end of one large message and the
start of another one
- {{OutboundMessageQueue}} - The race from CASSANDRA-15958 is fixed? (Noticed
the comment was removed...)
- {{InboundMessageHandler}} - The class-level JavaDoc is now almost identical
to {{AbstractMessageHandler}}, so we might want to narrow down to the places
where the former specializes. (ex. Only it uses, {{ProcessMessage}}.) The same
thing goes for the {{LargeMessage}} abstract class and its two sub-classes.
- {{CQLMessageHandler}} - could use brief class-level JavaDoc on how it extends
{{AbstractMessageHandler}}
- {{transport.Frame}} - brief class-level JavaDoc might help clarify its
scope/usage (even if it's renamed)
- Would class-level JavaDoc for {{transport.Dispatcher}} and
{{transport.Flusher}} be helpful. (ex. How do they interact with each other?)
- {{Payload#finish()}} - It might be useful to have a bit of detail in the
method JavaDoc explaining when we're expected to call it in the {{Payload}}
lifecycle.
- Do we need a deprecation plan for {{native_transport_frame_block_size_in_kb}}
...or not so much, because it was only added for 4.0 anyway?
*Zombies*
- {{transport.Frame.Header.BODY_LENGTH_SIZE}} is unused
- {{PasswordAuthenticatorTest}} - {{import java.net.InetSocketAddress}} is
unused
- {{OutboundConnection}} - {{import java.util.stream.Stream}} is unused
- {{StressSettings}} - {{import com.datastax.driver.core.Metadata}} and
{{import com.google.common.collect.ImmutableMap}} are unused
- {{FrameCompressor.LZ4Compressor}} - {{compress()}} doesn't need the
{{throws}} clause, and {{outputOffset + 0}} could be simplified
- {{ProtocolNegotiationTest}} - can remove the dead {{throws}} clauses from a
few tests
- {{ProtocolErrorTest}} - {{testErrorMessageWithNullString()}} doesn't need
throws clause
- {{CQLMessageHandler}} - {{UNKNOWN_STREAM_ID}} and {{import
org.apache.cassandra.net.Crc} are unused
- {{FrameEncoder.Payload#isEmpty()}} is unused
- {{InflightRequestPayloadTrackerTestGLOBAL_LIMITS}} is unused
- {{CQLConnectionTest}} - extra semicolon at the end of
{{handleErrorDuringNegotiation()}} and {{handleCorruptionAfterNegotiation()}},
and {{TestConsumer#scheduled}} is unused
- {{Frame.Decoder}} - {{decodeHeader}} and {{decodeFrame}} don't need a
{{throws}} clause
- {{Frame.Encoder}} - dead semicolon after private constructor
- {{ProtocolVersion}} - assignment of {{SUPPORTED}} has a redundant cast to
{{ProtocolVersion[]}}
*Naming*
- Maybe we can just call {{transport.Frame}} {{MessageFrame}} , given it
corresponds to a single CQL {{Message}}
- {{PipelineConfigurator}} - messageFrameDecoder/Encoder could be named
transportFrameDecoder/Encoder (unless the types under it are renamed, of course)
- WrappedException -> StreamAwareException?
- FrameSet -> FrameList
*Nits/Compiler Warnings/Minor Optimizations*
- {{transport.Frame#clone()}} is fine, but it might need
@SuppressWarnings("MethodDoesntCallSuperMethod")
- {{transport.Frame.Encoder#encode()}} needs {{@Override}} and doesn't need
{{throws}} clause
- {{transport.Frame.Header}} - Relying on the ordinals in {{Flag}} seems
brittle. What about just assigning the flags positions and using those in
{{serialize()}}?
- {{OptionsMessage}} - Not sure how many {{OPTIONS}} message we send, but the
{{supported}} {{HashMap}} in {{execute()}} could be sized exactly.
- {{SimpleClient}} - might be a good time to replace {{new
Slf4JLoggerFactory()}} w/ {{Slf4JLoggerFactory.INSTANCE}} in the {{static}}
block
- {{BufferPoolAllocator.Wrapped#deallocate()}} could probably just make the
null assignment in the body of the if statement
- {{AbstractMessageHandler}}
-- {{acquireCapacity()}} doesn't seem like it actually needs to suppress
"BooleanMethodIsAlwaysInverted"
-- break {{corruptFramesRecovered}}, {{corruptFramesUnrecovered}} (and the two
pairs of fields after) onto separate lines
-- some places in the JavaDoc read "...that are contained, like...", but that
should probably say "self-contained"
-- {{LargeMessage#releaseBuffers()}} and {{releaseBuffersAndCapacity()}} have
multiple statements on a line
- {{CQLMessagehandler}} - "whereever" -> "wherever" in the
{{handleErrorAndRelease()}} JavaDoc
- {{InflightRequestPayloadTrackerTest}} - a handful of tests here can use
try-with-resources for {{SimpleClient}}
- {{Flusher}} - We allocate a {{Payload}} and then trim it in a couple places.
We could factor out a little {{trimToMaxPayloadSize()}} method.
- {{Payload#setSelfContained()}} is never used with argument {{false}}
- {{PreV5Handlers.ProtocolDecoder#decode()}} should use {{<List<Object>}}
results instead of {{List results}}
- {{PreV5Handlers.ProtocolEncoder#encode()}} should use {{List<Object>}}
results instead of {{List results}}
- {{PreV5Handlers.ExceptionHandler#exceptionCaught()}} can defer init of
{{handler}} and {{errorMessage}} until we know the channel is open
- {{Dispatcher}} - could bind {{FlushItemConverter}} and {{FlushItem}} to the
same type
- {{DriverBurnTest}} - {{allocationObserver}} could be {{final}}
- {{PipelineConfigurator}} - {{WRITE_BUFFER_HIGH_WATER_MARK}} and
{{WRITE_BUFFER_LOW_WATER_MARK}} on {{ServerBootstrap}} are deprecated in favor
of passing a {{WriteBufferWaterMark}}
- {{FrameEncoderLZ4}}
-- extra line break between {{public}} and {{class FrameEncoderLZ4}}
-- might be able to factor a method out of the 8 lines or so starting with
{{CRC32 crc - crc32();}} in {{encode()}} and move up to {{FrameEncoder}}, then
reuse that method in {{FrameEncoderCrc#encode()}}
- {{Server.Builder#getSocket()}} could move it's {{return}} to the end/outside
the {{if}} and invert the condition/drop the {{else}}
*Questions*
- Looking at https://users.ece.cmu.edu/~koopman/crc/c32/0x82608edb_len.txt,
does a payload size of 128 KB with crc32 mean we have a HD=3 and will detect
all 1 and 2 bit errors?
- Is there anything behind the default stating capacity of {{FrameSet}} being 5?
> 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: Alex Petrov
> 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
