[
https://issues.apache.org/jira/browse/CASSANDRA-15299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17139920#comment-17139920
]
Sam Tunnicliffe edited comment on CASSANDRA-15299 at 6/18/20, 6:58 PM:
-----------------------------------------------------------------------
Pushed an updated branch where the protocol changes are pretty stable. I've
been testing this with the
[java2772|https://github.com/datastax/java-driver/tree/java2772] branch of the
driver and with debug-cql and everything is working pretty much as expected.
Due to current lack of support in the python driver, I've had to use a modified
[dtest branch|https://github.com/beobal/cassandra-dtest/tree/15299] and make a
temporary hack to cqlsh, but the tests that are running are pretty much green.
Obviously, those are not covering anything from v5 now, but my primary concern
was to make sure there's no regressions for v4 clients.
Although I think this is ready for some more eyes on it, there's still a
non-trivial amount of work to be done. Items outstanding include:
* Comprehensive unit and in-jvm tests - in progress
* Metrics
* Python driver support (doesn't have to be fully implemented, but a basic
level is needed for pytests and cqlsh)
* Documentation
* Renaming existing classes. There are a number of slightly confusing
conflicts in naming now. These should be simple to resolve, just automated
renaming mostly, but I've held off doing them for now because they'll make the
patch much bigger and probably harder to read.
The patch is also not quite a massive at it might appear at first. A large
proportion is the revert of CASSANDRA-13304, and the rest is largely additive.
I've tried hard not touch code on the v4 path, even where it could clearly be
refactored, to minimize the delta & the risk there. So the patch largely
consists of some v5 specific additions, moving a few existing classes/methods
around, and changing modifiers on previously private/package-private things.
I'm still actively working on the tests, but I don't think that (nor the
renaming) need hold up review any more.
|branch|[15299-trunk|https://github.com/beobal/cassandra/tree/15299-trunk]|
|dtests|[15299|https://github.com/beobal/cassandra-dtest/tree/15299]|
|ci|[circle|https://app.circleci.com/pipelines/github/beobal/cassandra?branch=15299-trunk]|
was (Author: beobal):
Pushed an updated branch where the protocol changes are pretty stable. I've
been testing this with the java2772 branch of the driver and with debug-cql and
everything is working pretty much as expected. Due to current lack of support
in the python driver, I've had to use a modified [dtest
branch|https://github.com/beobal/cassandra-dtest/tree/15299] and make a
temporary hack to cqlsh, but the tests that are running are pretty much green.
Obviously, those are not covering anything from v5 now, but my primary concern
was to make sure there's no regressions for v4 clients.
Although I think this is ready for some more eyes on it, there's still a
non-trivial amount of work to be done. Items outstanding include:
* Comprehensive unit and in-jvm tests - in progress
* Metrics
* Python driver support (doesn't have to be fully implemented, but a basic
level is needed for pytests and cqlsh)
* Documentation
* Renaming existing classes. There are a number of slightly confusing conflicts
in naming now. These should be simple to resolve, just automated renaming
mostly, but I've held off doing them for now because they'll make the patch
much bigger and probably harder to read.
The patch is also not quite a massive at it might appear at first. A large
proportion is the revert of CASSANDRA-13304, and the rest is largely additive.
I've tried hard not touch code on the v4 path, even where it could clearly be
refactored, to minimize the delta & the risk there. So the patch largely
consists of some v5 specific additions, moving a few existing classes/methods
around, and changing modifiers on previously private/package-private things.
I'm still actively working on the tests, but I don't think that (nor the
renaming) need hold up review any more.
|branch|[15299-trunk|https://github.com/beobal/cassandra/tree/15299-trunk]|
|dtests|[15299|https://github.com/beobal/cassandra-dtest/tree/15299]|
|ci|[circle|https://app.circleci.com/pipelines/github/beobal/cassandra?branch=15299-trunk]|
> 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
