Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Fix Version/s: 3.0.3, 3.2
    • Component/s: Coordination
    • Labels:
      None

      Description

      CASSANDRA-6230 is being implemented with compression in mind, but it's not going to be implemented by the original ticket.

      Adding it on top should be relatively straight-forward, and important, since there are several users in the wild that use compression interface for encryption purposes. DSE is one of them (but isn't the only one). Losing encryption capabilities would be a regression.

        Activity

        Hide
        bdeggleston Blake Eggleston added a comment -

        Here's my initial implementation: https://github.com/bdeggleston/cassandra/commits/9428

        A few things to note:

        • The behavior of HintsWriter.Session.append has been altered slightly. When the size of a hint is larger than the remaining buffer, the buffer is first flushed to disk, then the hint is written into the newly cleared buffer. Previously, both the hint and the previous data were flushed to disk. I did this to avoid writing very small compressed blocks to disk or having to concatenate the two byte buffers.
        • When opening readers, an uncompressed reader is always instantiated first, then upgraded to a compressed reader if the file is compressed. This is because the hints descriptor containing the compressor metadata is written to the head of the file, and it needs to be written uncompressed.
        Show
        bdeggleston Blake Eggleston added a comment - Here's my initial implementation: https://github.com/bdeggleston/cassandra/commits/9428 A few things to note: The behavior of HintsWriter.Session.append has been altered slightly. When the size of a hint is larger than the remaining buffer, the buffer is first flushed to disk, then the hint is written into the newly cleared buffer. Previously, both the hint and the previous data were flushed to disk. I did this to avoid writing very small compressed blocks to disk or having to concatenate the two byte buffers. When opening readers, an uncompressed reader is always instantiated first, then upgraded to a compressed reader if the file is compressed. This is because the hints descriptor containing the compressor metadata is written to the head of the file, and it needs to be written uncompressed.
        Hide
        JoshuaMcKenzie Joshua McKenzie added a comment -

        Feedback:

        • In CompressedChecksummedDataInput.reBufferStandard:L65, consider ICompressor.initialCompressedBufferLength rather than 5% allocation overhead.
        • HintsWriter.bufferWrites: I'm not crazy about us having a volatile increment in the critical path that's only used in a testing context.
        • In HintsCompressionTest.multiFlushAndDeserializeTest, bitshifting on init of bufferSize is unnecessary.
        • Would prefer HintsCompressionTest to exercise all available compressor types as a matter of general hygiene.

        Nits:

        • extra space in yaml before new param
        • extra space before closing brace in HintsWriter.create()
        • unused imports in HintsCompressionTest
        • some extra whitespace (doubled) in HintsCompressionTest.multiFlushAndDeserializeTest

        I'm fine with both the caveats you mention above. Looking good so far!

        Show
        JoshuaMcKenzie Joshua McKenzie added a comment - Feedback: In CompressedChecksummedDataInput.reBufferStandard:L65 , consider ICompressor.initialCompressedBufferLength rather than 5% allocation overhead. HintsWriter.bufferWrites : I'm not crazy about us having a volatile increment in the critical path that's only used in a testing context. In HintsCompressionTest.multiFlushAndDeserializeTest , bitshifting on init of bufferSize is unnecessary. Would prefer HintsCompressionTest to exercise all available compressor types as a matter of general hygiene. Nits: extra space in yaml before new param extra space before closing brace in HintsWriter.create() unused imports in HintsCompressionTest some extra whitespace (doubled) in HintsCompressionTest.multiFlushAndDeserializeTest I'm fine with both the caveats you mention above. Looking good so far!
        Hide
        bdeggleston Blake Eggleston added a comment -

        I've pushed up some additional commits to my branch addressing your points. Thanks for suggesting I exercise all compressor types, it uncovered some problems with the way I was allocating buffers, which I fixed.

        Show
        bdeggleston Blake Eggleston added a comment - I've pushed up some additional commits to my branch addressing your points. Thanks for suggesting I exercise all compressor types, it uncovered some problems with the way I was allocating buffers, which I fixed.
        Hide
        JoshuaMcKenzie Joshua McKenzie added a comment -

        +1, pending CI:

        branch testall dtest
        9428 testall dtest
        9428_trunk testall dtest
        Show
        JoshuaMcKenzie Joshua McKenzie added a comment - +1, pending CI: branch testall dtest 9428 testall dtest 9428_trunk testall dtest
        Hide
        JoshuaMcKenzie Joshua McKenzie added a comment -

        Committed. Thanks!

        Show
        JoshuaMcKenzie Joshua McKenzie added a comment - Committed . Thanks!

          People

          • Assignee:
            bdeggleston Blake Eggleston
            Reporter:
            iamaleksey Aleksey Yeschenko
            Reviewer:
            Joshua McKenzie
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development