-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZSTD snapshotting compression #2996
ZSTD snapshotting compression #2996
Conversation
❌ Gradle Check failure 626fa4d19ab09e7a5aa4bbc690e0aa5185a67858 |
a8209ef
to
a8c0b6a
Compare
❌ Gradle Check failure a8209ef5cd22284df5fccc51951a8d609287b875 |
❌ Gradle Check failure a8c0b6aed85759a7dd24362637846d060743ff1b |
a8c0b6a
to
32d2660
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cursory look at this looks good to me.
How does one use it?
I'd like @nknize to take a look please.
During creation of a repository set compression type (default is deflate, how it works now): {
...
"settings": {
"compress": true,
"compression_type": "zstd", // `deflate`
}
} |
@@ -182,4 +182,6 @@ grant { | |||
permission java.io.FilePermission "/sys/fs/cgroup/memory", "read"; | |||
permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read"; | |||
|
|||
// ZSTD permissions | |||
permission java.lang.RuntimePermission "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( Please fix that to have narrowed set of permissions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1; we can't have the security here be wide open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is a bit complex thing. What would be better:
- change it to
java.lang.RuntimePermission "loadLibrary.*"
- Alternative solution is to re-pack zstd library and add
*.so
files in thelibs
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good! Thanks for doing this. I left some comments.
I think we should add a none
compression_type
option to the DSL API that nulls out the compressor instead of adding a NullCompressor:
{
...
"settings": {
"compress": true,
"compression_type": "zstd", // `deflate`, `lz4`, `none`
}
}
We also need to widen the test coverage to include LZ4 and no compression.
* | ||
* @opensearch.internal | ||
*/ | ||
public class NullCompressor implements Compressor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompressorFactory.compressor
is nullable, why do we need an explicit null compressor? Can we just check compressor == null ? indexOutputOutputStream : compressor.threadLocalOutputStream(indexOutputOutputStream)
and do away with this empty class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH it is just a habit to use Null class and do not check it on null every where, but I think with None it would be better
// It needs to be different from other compressors and to not be specific | ||
// enough so that no stream starting with these bytes could be detected as | ||
// a XContent | ||
private static final byte[] HEADER = new byte[] { 'Z', 'S', 'T', 'D', '\0' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readability nit:
private static final byte[] HEADER = new byte[] { 'Z', 'S', 'T', 'D', '\0' }; | |
public static final String NAME = "ZSTD"; | |
private static final byte[] HEADER =NAME.getBytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I just wanted to do it as it done for DEFLATE { 'D', 'F', 'L', '\0' }
not sure about \0
@@ -182,4 +182,6 @@ grant { | |||
permission java.io.FilePermission "/sys/fs/cgroup/memory", "read"; | |||
permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read"; | |||
|
|||
// ZSTD permissions | |||
permission java.lang.RuntimePermission "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1; we can't have the security here be wide open
final boolean compress = randomBoolean(); | ||
settingsBuilder.put("compress", compress); | ||
if (compress) { | ||
settingsBuilder.put("compression_type", randomFrom(CompressorType.values())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't test no compression or LZ4 since CompressorType
doesn't contain those values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning to add LZ4 as a separate PR if you do not mind.
test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java
Show resolved
Hide resolved
Yes |
32d2660
to
fe6afd7
Compare
Gradle Check (Jenkins) Run Completed with:
|
@willyborankin Hey, would you like to address the gradle check failure and merge this PR? |
@Poojita-Raj I would like to. Im waiting for this one #3577 since it uses the same native libraries. |
@willyborankin Have you ever benchmarked this to measure the actual impact on snapshotting and restore? |
HI @willyborankin, I know you are working on a lot of things but wanted to see what you needed to help move this forward. Let me know. |
fe6afd7
to
1237624
Compare
server/src/main/java/org/opensearch/common/compress/ZstdCompressor.java
Outdated
Show resolved
Hide resolved
54e90ae
to
406865b
Compare
Gradle Check (Jenkins) Run Completed with:
|
Open an issue in that repo so we don't forget. |
Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io>
406865b
to
b8efda1
Compare
Gradle Check (Jenkins) Run Completed with:
|
@nknize you good with this? needs you to dismiss your review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had a chance to revisit this, but it seems we have enough recent reviews and we can patch and/or revert hidden dragons. So I'll unblock.
@nknize mind to resolve your comments as well (may be something pops up)? (the merge is still blocked) |
I would love to. But apparently you can't resolve outdated (missing) conversations. And the alleged "workaround" doesn't work on mobile view. I'm happy to merge as admin to override these pearly github gates. |
Merging as admin due to "outdated" conversation resolution bug. |
Thanks @willyborankin for your contribution and to everyone for your tenacity on this long running PR. |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2996-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4df347c6f904942b073ee4bc76ec87a095cee4c7
# Push it to GitHub
git push --set-upstream origin backport/backport-2996-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x Then, create a pull request where the |
@willyborankin mind sending manual backport against |
sure will do |
Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io>
Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io>
Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io>
…search-project#7906) Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io>
Changes: - Added ZSTD compressor for snapshotting - 2 JSON repository settings: - readonly - compression were moved into the BlobStoreRepository class and removed from other repos classes where they were used. Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Description
This PR adds support ZSTD compression for snapshoting metadata. ZSTD compression for indexes is out of scope since such compression must be supported by Lucene.
There is a PR in Lucene apache/lucene#439 with support of ZSTD, but they do not want to merge it so far.
Issues Resolved
#2192
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.