[ 
https://issues.apache.org/jira/browse/CASSANDRA-13304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16599314#comment-16599314
 ] 

Jordan West edited comment on CASSANDRA-13304 at 9/1/18 3:06 PM:
-----------------------------------------------------------------

[~beobal] I'm still going over the changes but I wanted to post this bug now 
before I finish. The issue is that the checksum over the lengths is only 
calculated over the least-significant byte of the compressed and uncompressed 
lengths. This means that if we introduce corruption in the three most 
significant bytes we won't catch it, which leads to a host of different bugs 
(index out of bounds exception , lz4 decompression issues, etc). I pushed a 
patch 
[here|https://github.com/jrwest/cassandra/commit/e57a2508c26f05efb826a7f4342964fa6d6691bd]
 with a new test that catches the issue. I left in the fixed seed for now so 
its easy for you to run and see the failing example in a debugger (its the 
second example that fails). I've pasted a stack trace of the failure with that 
seed below. The example generated has a single byte input and introduces 
corruption into the 4th byte in the stream (the second most significant byte of 
the first length in the stream). This leads to a case where the checksums match 
but when we go to read the data we read past the total length of the buffer.

A couple other comments while I'm here. All minor:
 * [~djoshi3] and I were reviewing the bug above before I posted and we were 
thinking it would be nice to refactor 
{{ChecksummingTransformer#transformInbound/transformOutbound}}. They are a bit 
large/unwieldy right now. We can open a JIRA to address this later if you 
prefer.
 * Re: the {{roundTripZeroLength}} property. This is mostly covered by the 
property I already added although this makes it more likely to generate a few 
cases. If you want to keep it I would recommend setting {{withExamples}} and 
using something small like 10 or 20 examples (since the state space is small).
 * The {{System.out.println}} I added in {{roundTripSafetyProperty}} should be 
removed

Stack Trace:
{code:java}
java.lang.AssertionError: Property falsified after 2 example(s) 
Smallest found falsifying value(s) :- \{(c,3), 0, null, Adler32}

Cause was :-
 java.lang.IndexOutOfBoundsException: readerIndex(10) + length(16711681) 
exceeds writerIndex(15): UnpooledHeapByteBuf(ridx: 10, widx: 15, cap: 54/54)
 at 
io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1401)
 at 
io.netty.buffer.AbstractByteBuf.checkReadableBytes(AbstractByteBuf.java:1388)
 at io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:870)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformer.transformInbound(ChecksummingTransformer.java:289)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.roundTripWithCorruption(ChecksummingTransformerTest.java:106)
 at 
org.quicktheories.dsl.TheoryBuilder4.lambda$checkAssert$9(TheoryBuilder4.java:163)
 at org.quicktheories.dsl.TheoryBuilder4.lambda$check$8(TheoryBuilder4.java:151)
 at org.quicktheories.impl.Property.tryFalsification(Property.java:23)
 at org.quicktheories.impl.Core.shrink(Core.java:111)
 at org.quicktheories.impl.Core.run(Core.java:39)
 at org.quicktheories.impl.TheoryRunner.check(TheoryRunner.java:35)
 at org.quicktheories.dsl.TheoryBuilder4.check(TheoryBuilder4.java:150)
 at org.quicktheories.dsl.TheoryBuilder4.checkAssert(TheoryBuilder4.java:162)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.corruptionCausesFailure(ChecksummingTransformerTest.java:87)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
 at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
 at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
 at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
 at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
 at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
 at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
 at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
 at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
 at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
 at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
 at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
 at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
 at 
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
 at 
com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
 at 
com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
 at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

Other found falsifying value(s) :-

{(c,3), 0, null, CRC32} \{(c,3), 1, null, CRC32} \{(c,3), 9, null, CRC32} 
\{(c,3), 11, null, CRC32} \{(c,3), 36, null, CRC32} \{(c,3), 50, null, CRC32} 
\{(c,3), 74, null, CRC32} \{(c,3), 99, null, CRC32}

Seed was 179207634899674

at org.quicktheories.core.ExceptionReporter.falsify(ExceptionReporter.java:43)
 at 
org.quicktheories.core.ExceptionReporter.falisification(ExceptionReporter.java:37)
 at 
org.quicktheories.impl.TheoryRunner.reportFalsification(TheoryRunner.java:48)
 at org.quicktheories.impl.TheoryRunner.check(TheoryRunner.java:37)
 at org.quicktheories.dsl.TheoryBuilder4.check(TheoryBuilder4.java:150)
 at org.quicktheories.dsl.TheoryBuilder4.checkAssert(TheoryBuilder4.java:162)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.corruptionCausesFailure(ChecksummingTransformerTest.java:87)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
 at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
 at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
 at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
 at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
 at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
 at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
 at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
 at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
 at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
 at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
 at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
 at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
 at 
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
 at 
com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
 at 
com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
 at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
 {code}


was (Author: jrwest):
[~beobal] I'm still going over the changes but I wanted to post this bug now 
before I finish. The issue is that the checksum over the lengths is only 
calculated over the least-significant byte of the compressed and uncompressed 
lengths. This means that if we introduce corruption in the three most 
significant bytes we won't catch it, which leads to a host of different bugs 
(index out of bounds exception , lz4 decompression issues, etc). I pushed a 
patch 
[here|https://github.com/jrwest/cassandra/commit/e57a2508c26f05efb826a7f4342964fa6d6691bd]
 with a new test that catches the issue. I left in the fixed seed for now so 
its easy for you to run and see the failing example in a debugger (its the 
second example that fails). I've pasted a stack trace of the failure with that 
seed below. The example generated has a single byte input and introduces 
corruption into the 3rd byte in the stream (the most significant byte of the 
first length in the stream). This leads to a case where the checksums match but 
when we go to read the data we read past the total length of the buffer.

A couple other comments while I'm here. All minor:
 * [~djoshi3] and I were reviewing the bug above before I posted and we were 
thinking it would be nice to refactor 
{{ChecksummingTransformer#transformInbound/transformOutbound}}. They are a bit 
large/unwieldy right now. We can open a JIRA to address this later if you 
prefer.
 * Re: the {{roundTripZeroLength}} property. This is mostly covered by the 
property I already added although this makes it more likely to generate a few 
cases. If you want to keep it I would recommend setting {{withExamples}} and 
using something small like 10 or 20 examples (since the state space is small).
 * The {{System.out.println}} I added in {{roundTripSafetyProperty}} should be 
removed

Stack Trace:
{code:java}
java.lang.AssertionError: Property falsified after 2 example(s) 
Smallest found falsifying value(s) :- \{(c,3), 0, null, Adler32}

Cause was :-
 java.lang.IndexOutOfBoundsException: readerIndex(10) + length(16711681) 
exceeds writerIndex(15): UnpooledHeapByteBuf(ridx: 10, widx: 15, cap: 54/54)
 at 
io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1401)
 at 
io.netty.buffer.AbstractByteBuf.checkReadableBytes(AbstractByteBuf.java:1388)
 at io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:870)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformer.transformInbound(ChecksummingTransformer.java:289)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.roundTripWithCorruption(ChecksummingTransformerTest.java:106)
 at 
org.quicktheories.dsl.TheoryBuilder4.lambda$checkAssert$9(TheoryBuilder4.java:163)
 at org.quicktheories.dsl.TheoryBuilder4.lambda$check$8(TheoryBuilder4.java:151)
 at org.quicktheories.impl.Property.tryFalsification(Property.java:23)
 at org.quicktheories.impl.Core.shrink(Core.java:111)
 at org.quicktheories.impl.Core.run(Core.java:39)
 at org.quicktheories.impl.TheoryRunner.check(TheoryRunner.java:35)
 at org.quicktheories.dsl.TheoryBuilder4.check(TheoryBuilder4.java:150)
 at org.quicktheories.dsl.TheoryBuilder4.checkAssert(TheoryBuilder4.java:162)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.corruptionCausesFailure(ChecksummingTransformerTest.java:87)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
 at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
 at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
 at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
 at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
 at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
 at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
 at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
 at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
 at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
 at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
 at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
 at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
 at 
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
 at 
com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
 at 
com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
 at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

Other found falsifying value(s) :-

{(c,3), 0, null, CRC32} \{(c,3), 1, null, CRC32} \{(c,3), 9, null, CRC32} 
\{(c,3), 11, null, CRC32} \{(c,3), 36, null, CRC32} \{(c,3), 50, null, CRC32} 
\{(c,3), 74, null, CRC32} \{(c,3), 99, null, CRC32}

Seed was 179207634899674

at org.quicktheories.core.ExceptionReporter.falsify(ExceptionReporter.java:43)
 at 
org.quicktheories.core.ExceptionReporter.falisification(ExceptionReporter.java:37)
 at 
org.quicktheories.impl.TheoryRunner.reportFalsification(TheoryRunner.java:48)
 at org.quicktheories.impl.TheoryRunner.check(TheoryRunner.java:37)
 at org.quicktheories.dsl.TheoryBuilder4.check(TheoryBuilder4.java:150)
 at org.quicktheories.dsl.TheoryBuilder4.checkAssert(TheoryBuilder4.java:162)
 at 
org.apache.cassandra.transport.frame.checksum.ChecksummingTransformerTest.corruptionCausesFailure(ChecksummingTransformerTest.java:87)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
 at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
 at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
 at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
 at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
 at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
 at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
 at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
 at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
 at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
 at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
 at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
 at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
 at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
 at 
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
 at 
com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
 at 
com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
 at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
 {code}

> Add checksumming to the native protocol
> ---------------------------------------
>
>                 Key: CASSANDRA-13304
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-13304
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Core
>            Reporter: Michael Kjellman
>            Assignee: Sam Tunnicliffe
>            Priority: Blocker
>              Labels: client-impacting
>             Fix For: 4.x
>
>         Attachments: 13304_v1.diff, boxplot-read-throughput.png, 
> boxplot-write-throughput.png
>
>
> The native binary transport implementation doesn't include checksums. This 
> makes it highly susceptible to silently inserting corrupted data either due 
> to hardware issues causing bit flips on the sender/client side, C*/receiver 
> side, or network in between.
> Attaching an implementation that makes checksum'ing mandatory (assuming both 
> client and server know about a protocol version that supports checksums) -- 
> and also adds checksumming to clients that request compression.
> The serialized format looks something like this:
> {noformat}
>  *                      1 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
>  *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |  Number of Compressed Chunks  |     Compressed Length (e1)    /
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * /  Compressed Length cont. (e1) |    Uncompressed Length (e1)   /
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * | Uncompressed Length cont. (e1)| CRC32 Checksum of Lengths (e1)|
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * | Checksum of Lengths cont. (e1)|    Compressed Bytes (e1)    +//
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                      CRC32 Checksum (e1)                     ||
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                    Compressed Length (e2)                     |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                   Uncompressed Length (e2)                    |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                CRC32 Checksum of Lengths (e2)                 |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                     Compressed Bytes (e2)                   +//
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                      CRC32 Checksum (e2)                     ||
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                    Compressed Length (en)                     |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                   Uncompressed Length (en)                    |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                CRC32 Checksum of Lengths (en)                 |
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                      Compressed Bytes (en)                  +//
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  * |                      CRC32 Checksum (en)                     ||
>  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
> {noformat}
> The first pass here adds checksums only to the actual contents of the frame 
> body itself (and doesn't actually checksum lengths and headers). While it 
> would be great to fully add checksuming across the entire protocol, the 
> proposed implementation will ensure we at least catch corrupted data and 
> likely protect ourselves pretty well anyways.
> I didn't go to the trouble of implementing a Snappy Checksum'ed Compressor 
> implementation as it's been deprecated for a while -- is really slow and 
> crappy compared to LZ4 -- and we should do everything in our power to make 
> sure no one in the community is still using it. I left it in (for obvious 
> backwards compatibility aspects) old for clients that don't know about the 
> new protocol.
> The current protocol has a 256MB (max) frame body -- where the serialized 
> contents are simply written in to the frame body.
> If the client sends a compression option in the startup, we will install a 
> FrameCompressor inline. Unfortunately, we went with a decision to treat the 
> frame body separately from the header bits etc in a given message. So, 
> instead we put a compressor implementation in the options and then if it's 
> not null, we push the serialized bytes for the frame body *only* thru the 
> given FrameCompressor implementation. The existing implementations simply 
> provide all the bytes for the frame body in one go to the compressor 
> implementation and then serialize it with the length of the compressed bytes 
> up front.
> Unfortunately, this won't work for checksum'ing for obvious reasons as we 
> can't naively just checksum the entire (potentially) 256MB frame body and 
> slap it at the end... so,
> The best place to start with the changes is in {{ChecksumedCompressor}}. I 
> implemented one single place to perform the checksuming (and to support 
> checksuming) the actual required chunking logic. Implementations of 
> ChecksumedCompressor only implement the actual calls to the given compression 
> algorithm for the provided bytes.
> Although the interface takes a {{Checksum}}, right now the attached patch 
> uses CRC32 everywhere. As of right now, given JDK8+ has support for doing the 
> calculation with the Intel instruction set, CRC32 is about as fast as we can 
> get right now.
> I went with a 32kb "default" for the chunk size -- meaning we will chunk the 
> entire frame body into 32kb chunks, compress each one of those chunks, and 
> checksum the chunk. Upon discussing with a bunch of people and researching 
> how checksums actually work and how much data they will protect etc -- if we 
> use 32kb chunks with CRC32 we can catch up to 32 bits flipped in a row (but 
> more importantly catch the more likely corruption where a single bit is 
> flipped) with pretty high certainty. 64kb seems to introduce too much of a 
> probability of missing corruption.
> The maximum block size LZ4 operates on is a 64kb chunk -- so this combined 
> with the need to make sure the CRC32 checksums are actually going to catch 
> stuff -- chunking at 32kb seemed like a good reasonable value to use when 
> weighing both checksums and compression (to ensure we don't kill our 
> compression ratio etc).
> I'm not including client changes here -- I asked around and I'm not really 
> sure what the policy there is -- do we update the python driver? java driver? 
> how has the timing of this stuff been handled in the past?



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to