clintropolis commented on code in PR #12408:
URL: https://github.com/apache/druid/pull/12408#discussion_r880298649
##########
processing/src/main/java/org/apache/druid/segment/data/CompressionStrategy.java:
##########
@@ -344,6 +360,81 @@ public ByteBuffer compress(ByteBuffer in, ByteBuffer out)
}
}
+ public static class ZstdCompressor extends Compressor
+ {
+ private static final ZstdCompressor DEFAULT_COMPRESSOR = new
ZstdCompressor();
+
+ @Override
+ ByteBuffer allocateInBuffer(int inputSize, Closer closer)
+ {
+ ByteBuffer inBuffer = ByteBuffer.allocateDirect(inputSize);
+ closer.register(() -> ByteBufferUtils.free(inBuffer));
+ return inBuffer;
+ }
+
+ @Override
+ ByteBuffer allocateOutBuffer(int inputSize, Closer closer)
+ {
+ ByteBuffer outBuffer = ByteBuffer.allocateDirect((int)
Zstd.compressBound(inputSize));
+ closer.register(() -> ByteBufferUtils.free(outBuffer));
+ return outBuffer;
+ }
+
+ @Override
+ public ByteBuffer compress(ByteBuffer in, ByteBuffer out)
+ {
+ int position = in.position();
+ out.clear();
+ long sizeNeeded = Zstd.compressBound(in.remaining());
+ if (out.remaining() < sizeNeeded) {
+ throw new RuntimeException("Output buffer too small, please allocate
more space. " + sizeNeeded + " required.");
+ }
+ Zstd.compress(out, in, Zstd.maxCompressionLevel());
+ in.position(position);
+ out.flip();
+ return out;
+ }
+ }
+
+ public static class ZstdDecompressor implements Decompressor
+ {
+ private static final ZstdDecompressor DEFAULT_COMPRESSOR = new
ZstdDecompressor();
+
+ @Override
+ public void decompress(ByteBuffer in, int numBytes, ByteBuffer out)
+ {
+ out.clear();
+ // some tests don't use dbb's and zstd jni doesn't allow for non-dbb
byte buffers.
Review Comment:
apologies for the delay; thinking a bit more about it, do we actually need
to modify `CompressionStrategyTest` now that we can handle heap buffers? It
might be slightly better coverage to leave the test added by #12521 instead of
just testing direct. That said, the changes overall lgtm now :+1:
looks like there are a couple of CI failures about pom dependency stuffs
https://app.travis-ci.com/github/apache/druid/jobs/571153862#L3183
and also maybe our license checking scripts aren't handling an entry
correctly, though it isn't obvious why yet:
https://app.travis-ci.com/github/apache/druid/jobs/571153859#L4257
since the script has that key...
https://github.com/apache/druid/blob/master/distribution/bin/check-licenses.py#L236
maybe its a bad error message though, due to
https://github.com/apache/druid/blob/master/licenses.yaml#L3967 not being
updated to the new version in this PR?
https://github.com/apache/druid/blob/master/licenses.yaml#L3979 should also be
updated to whatever version of zstd that the new version of the jni wrapper
points to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
