ZSTD snapshotting compression by willyborankin · Pull Request #2996 · opensearch-project/OpenSearch

willyborankin · 2022-04-20T13:29:21Z

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

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

opensearch-ci-bot · 2022-04-20T13:53:42Z

❌ Gradle Check failure 626fa4d19ab09e7a5aa4bbc690e0aa5185a67858
Log 4638

Reports 4638

opensearch-ci-bot · 2022-05-27T22:26:33Z

❌ Gradle Check failure a8209ef5cd22284df5fccc51951a8d609287b875
Log 5637

Reports 5637

opensearch-ci-bot · 2022-05-27T22:26:49Z

❌ Gradle Check failure a8c0b6aed85759a7dd24362637846d060743ff1b
Log 5638

Reports 5638

opensearch-ci-bot · 2022-05-28T10:21:14Z

✅ Gradle Check success 32d2660
Log 5646

Reports 5646

dblock

A cursory look at this looks good to me.

How does one use it?

I'd like @nknize to take a look please.

willyborankin · 2022-06-14T13:05:41Z

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`
   }
}

reta · 2022-06-23T23:32:11Z

server/src/main/resources/org/opensearch/bootstrap/security.policy

  permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read";

+  // ZSTD permissions
+  permission java.lang.RuntimePermission "*";


:( Please fix that to have narrowed set of permissions

+1; we can't have the security here be wide open

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 the libs folder?

nknize

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.

nknize · 2022-06-29T16:27:18Z

server/src/main/java/org/opensearch/common/compress/NullCompressor.java

+ *
+ * @opensearch.internal
+ */
+public class NullCompressor implements Compressor {


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?

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

nknize · 2022-06-29T16:35:55Z

server/src/main/java/org/opensearch/common/compress/ZstdCompressor.java

+    // 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' };


readability nit:

Suggested change

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();

Well I just wanted to do it as it done for DEFLATE { 'D', 'F', 'L', '\0' } not sure about \0

nknize · 2022-06-29T16:37:18Z

server/src/main/resources/org/opensearch/bootstrap/security.policy

  permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read";

+  // ZSTD permissions
+  permission java.lang.RuntimePermission "*";


+1; we can't have the security here be wide open

nknize · 2022-06-29T16:47:17Z

...n/java/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java

+        final boolean compress = randomBoolean();
+        settingsBuilder.put("compress", compress);
+        if (compress) {
+            settingsBuilder.put("compression_type", randomFrom(CompressorType.values()));


This doesn't test no compression or LZ4 since CompressorType doesn't contain those values.

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

willyborankin · 2022-07-07T18:12:01Z

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.

Yes none much better than Null will change

github-actions · 2022-10-23T22:06:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5323/
CommitID: fe6afd7

Poojita-Raj · 2023-03-13T19:22:57Z

@willyborankin Hey, would you like to address the gradle check failure and merge this PR?

willyborankin · 2023-03-14T16:31:18Z

@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.

wbeckler · 2023-04-26T17:34:23Z

@willyborankin Have you ever benchmarked this to measure the actual impact on snapshotting and restore?

stephen-crawford · 2023-05-17T13:51:49Z

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.

server/src/main/java/org/opensearch/common/compress/ZstdCompressor.java

github-actions · 2023-05-23T20:43:57Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/16101/
CommitID: 406865b

dblock · 2023-05-23T21:28:14Z

FYI, there is only one plugin (outside core) that implements repository for OCI [1], it may need minor adjustments (primarily, to use the settings centralized in BlobStoreRepository), otherwise the change should not break anything.
[1] https://github.com/opensearch-project/opensearch-oci-object-storage/

good point should I connect with them?

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>

github-actions · 2023-06-01T10:12:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

URL: https://build.ci.opensearch.org/job/gradle-check/16645/
CommitID: b8efda1
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

dblock · 2023-06-01T12:59:02Z

@nknize you good with this? needs you to dismiss your review

nknize

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.

reta · 2023-06-01T14:23:16Z

@nknize mind to resolve your comments as well (may be something pops up)? (the merge is still blocked)

nknize · 2023-06-01T14:38:35Z

@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.

nknize · 2023-06-01T14:40:05Z

Merging as admin due to "outdated" conversation resolution bug.

nknize · 2023-06-01T14:41:41Z

Thanks @willyborankin for your contribution and to everyone for your tenacity on this long running PR.

opensearch-trigger-bot · 2023-06-01T14:50:41Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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 base branch is 2.x and the compare/head branch is backport/backport-2996-to-2.x.

reta · 2023-06-01T14:51:26Z

@willyborankin mind sending manual backport against 2.x? thank you

willyborankin · 2023-06-01T16:56:29Z

@willyborankin mind sending manual backport against 2.x? thank you

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>

…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>

willyborankin force-pushed the zstdsnapshoting-compression branch 2 times, most recently from a8209ef to a8c0b6a Compare May 27, 2022 21:08

willyborankin force-pushed the zstdsnapshoting-compression branch from a8c0b6a to 32d2660 Compare May 28, 2022 09:46

willyborankin marked this pull request as ready for review May 28, 2022 10:24

willyborankin requested review from a team and reta as code owners May 28, 2022 10:24

willyborankin changed the title ~~Zstdsnapshoting compression~~ ZSTD snapshotting compression May 28, 2022

willyborankin mentioned this pull request May 28, 2022

Introduce more compression libraries and implementations #3354

Open

dblock requested review from andrross, mch2 and nknize June 13, 2022 23:43

dblock reviewed Jun 13, 2022

View reviewed changes

willyborankin mentioned this pull request Jun 14, 2022

Add experimental support for zstd compression. #3577

Merged

5 tasks

reta reviewed Jun 23, 2022

View reviewed changes

nknize suggested changes Jun 29, 2022

View reviewed changes

willyborankin force-pushed the zstdsnapshoting-compression branch from 32d2660 to fe6afd7 Compare October 23, 2022 20:39

willyborankin force-pushed the zstdsnapshoting-compression branch from fe6afd7 to 1237624 Compare May 18, 2023 16:35

willyborankin requested review from Bukhtawar and anasalkouz as code owners May 18, 2023 16:35

reta reviewed May 23, 2023

View reviewed changes

server/src/main/java/org/opensearch/common/compress/ZstdCompressor.java Outdated Show resolved Hide resolved

willyborankin force-pushed the zstdsnapshoting-compression branch from 54e90ae to 406865b Compare May 23, 2023 20:13

dblock approved these changes May 23, 2023

View reviewed changes

dblock requested a review from nknize May 23, 2023 21:29

willyborankin force-pushed the zstdsnapshoting-compression branch from 406865b to b8efda1 Compare June 1, 2023 09:40

willyborankin requested a review from dbwiddis as a code owner June 1, 2023 09:40

nknize approved these changes Jun 1, 2023

View reviewed changes

nknize merged commit 4df347c into opensearch-project:main Jun 1, 2023

reta added the backport 2.x Backport to 2.x branch label Jun 1, 2023

cwperks mentioned this pull request Jun 2, 2023

Match version of zstd-jni from core opensearch-project/security#2832

Merged

3 tasks

willyborankin mentioned this pull request Jun 4, 2023

[Backport 2.x] Add ZSTD compression for snapshotting (#2996) #7906

Merged

willyborankin mentioned this pull request Jun 5, 2023

Remove COMPRESSOR variable from CompressorFactory #7907

Merged

6 tasks

	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();

Conversation

willyborankin commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Apr 20, 2022

Uh oh!

opensearch-ci-bot commented May 27, 2022

Uh oh!

opensearch-ci-bot commented May 27, 2022

Uh oh!

opensearch-ci-bot commented May 28, 2022

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

willyborankin commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willyborankin Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willyborankin Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willyborankin commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Poojita-Raj commented Mar 13, 2023

Uh oh!

willyborankin commented Mar 14, 2023

Uh oh!

wbeckler commented Apr 26, 2023

Uh oh!

stephen-crawford commented May 17, 2023

Uh oh!

Uh oh!

github-actions bot commented May 23, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented May 23, 2023

Uh oh!

github-actions bot commented Jun 1, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented Jun 1, 2023

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

reta commented Jun 1, 2023

Uh oh!

nknize commented Jun 1, 2023

willyborankin commented Apr 20, 2022 •

edited

Loading

willyborankin commented Jun 14, 2022 •

edited

Loading

willyborankin Jul 7, 2022 •

edited

Loading

willyborankin Jul 7, 2022 •

edited

Loading

willyborankin commented Jul 7, 2022 •

edited

Loading