[ https://issues.apache.org/jira/browse/CASSANDRA-15299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17211482#comment-17211482 ]
Caleb Rackliffe edited comment on CASSANDRA-15299 at 10/10/20, 12:28 AM: ------------------------------------------------------------------------- [~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? was (Author: maedhroz): [~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