-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Demote] ZStd compression from GA / LTS to experimental and release a 2.9.1 patch #9422
Comments
@nknize we cannot add new features in patch release (i.e 2.9.1) rather only bug fix and security patches as per SemVar guidelines. We can definitely get the bug fix in but I am not sure about adding experimental flag since that might be considered a feature, so we can target those changes only for next minor release. |
@bbarani Unfortunately we can't allow ZStd to remain available by default because rolling this back requires users to reindex if they chose these codecs. We have to revert this feature ASAP before users start indexing. This was mistakenly released GA, hence the patch. |
Regarding the option to revert #7908, I just want to note that will leave any users who indexed data with zstd using 2.9.0 in a bad spot. Their only option would be to reindex that data with 2.9.0 before upgrading to 2.9.1, since the 2.9.1 release will not contain the sandbox plugin functionality. |
Great point @andrross! I'm adding context from follow on discussion from slack just so we don't lose it.
I added this as an alternative to discuss. |
@nknize - What is the general guideline for any native dependency? Are you saying OpenSearch will never support native dependencies going forward?
What is the intent of this? By moving to optional are you suggesting users to use Zstd at their own risk? Zstd is providing 30% reduction in storage size and latencies as good as best_speed. What is the alternative to this? |
As plugins, sure. But likely not as default modules. Plugins are just like modules except users have to opt in by installing them.
Depends on how you define "risk". ldd errors due to libc issues is certainly a "risk". An alternative to the jni implementation would be to use the pure java solution. Then we could include it as a module without portability risks.
That's only part of the picture. Retrieval time per 10k docs is nearly five times slower than BEST_SPEED. So it's not without its tradeoffs. |
@nknize @backslasht sharing some search numbers from my runs with NYC Taxis Cluster Configuration:
|
@sarthakaggarwal97 - Can you add details about the cluster configuration for the run? |
Fair point! |
I've updated it in my previous comment alongside the numbers, thanks @backslasht |
@sarthakaggarwal97 sadly the latency charts do not reflect memory leaks (especially the native one)
@nknize afaik the pure Java implementation exists and was evaluated with terrible performance (comparing to ZSTD-jni) |
@sarthakaggarwal97 can you add two things to your nyc_taxi benchmark run:
The reasoning is: regressions have historically occurred when decompressing stored fields and I don't see any stored fields in the nyc_taxis workload. We should make this part of the regular benchmark to prevent us from missing another regression.
@reta Do you have that benchmark handy? I haven't dug around for it and the AirCompressor README touts "...typically 10-40% faster than the JNI wrapper for the native libraries." The only thing I quickly dug up was the comment justifying the purpose of the library, which aligns with our purpose of avoiding strict native dependencies. @sarthakaggarwal97 It's also worth running luceneutil's |
Hm ... I definitely seen somewhere, there are mentions here apache/lucene#9784 (comment), and here #3354 for LZ4 (not applicable to ZSTD). There are benchmarks available in the repo, I will run them: https://github.com/airlift/aircompressor/blob/master/src/test/java/io/airlift/compress/benchmark/CompressionBenchmark.java |
@nknize Shouldn't |
Sorry this issue was closed automatically after #9431 |
@mgodwan For queries, |
@nknize I've run aircompressor benchmarks locally on my machine ( i7-10750H × 12, 64Gb, Linux): Here are the compression mem throughput:
vs
Same for decompression:
vs
As per this numbers, |
@reta what is the fourth column? |
@reta The differences are:
|
+1. Sounds good to me. |
this is a very smart idea, @andrross, solves for semver for the default distribution with plugins at least |
Thank you @andrross , much appreciated |
I'm just catching up on this after being out. In the spirit of full transparency to the community there has been a lot of back channeled and closed door discussions around this topic that should really take place in the open for traceability and community involvement. With that I have a few comments and questions:
They are not. Beating a dead horse described and defined in #5910, modules are installed by default and cannot be uninstalled.
?? I'm confused by this. Are you proposing switching the features in the
It appears, per @dblock comment, that somewhere in the bundle assemble workflow all downstream OpenSearch (ODFE) developed plugins (external repos) are in fact installed by default. Looks like that's a relic of the original ODFE days. IMO that's a strange decision with interesting (and potentially harmful) side effects... but that's a separate conversation. With respect to this issue +1 to move zstd compression to a plugin in 2.10 per the original proposal. However, it sounds like you're suggesting we update the bundle build of the "ecosystem" artifacts to install the new core |
@nknize Wow this was horrible phrasing/typo and I'm absolutely not proposing to change the behavior of modules. I have updated the comment in place as well, but what I meant to say is that in my proposal the zstd compression plugin would not be installed by default in the min distribution, which it would be if it were made into a module. |
Phew. +1 Just going to re-iterate my suggestion Update: for sake of time it would be quicker to move it to common-util. This way we wouldn't have to wait for a new repo to be approved and created by the "Admin" group. |
@nknize I like this idea. I know you don't love the "install plugin in distribution by default" behavior but it does allow us to move plugins around without changing the end user experience. Update: just saw the comment about moving to |
+1 to moving out of core to separate repo |
+1, I agree lets not pollute common-utils just because it is easier to move it there.
This can be different plugins for different compressions and the users are free to choose what they like (zstd, snappy). Not sure, if bundling everything together is a good idea. |
I agree with this in the long term. The problem I worry about is the time it will take to get a new repo requested -> approved -> created. @hyandell any idea how long this will take? I know it will have to go through trademark checks and all that. We have feature release scheduled for 2.10 on 9/19, a little over two weeks. If you think we can't get the repo created in the next couple of days let us know. Any later than that won't give us enough time to onboard a new repo to the bundle. If that's the case then I suggest putting it in common-utils until the new repo can be created and everything moved without rushing. |
Great, I'm really looking forward to it. |
I've seen empty repos created in ~24h. |
It has been like this since OpenSearch v1, and is not a relic. Most users want(ed) a fully featured OpenSearch with security enabled. This is done in the bundle assemble workflow. |
Markdowns don't tell me anything. Code does.
+1 for security by default. -1 to install every OpenSearch plugin known to man (or at least in |
@reta That's my understanding, we will not release 2.9.1 |
I believe changes are required in the distribution assembly logic to make the "install by default" thing actually happen. I have opened an issue here: opensearch-project/opensearch-build#3971 |
Is your feature request related to a problem? Please describe.
ZStd was introduced as an experimental compression option for Lucene indexes in #3577. This brought the implementation as a
module
(installed by default) under thesandbox
directory. However, the feature would remain "expert" optional as it would only be installed if users built the distribution themselves and passedsandbox.enabled=true
at JVM startup.Unfortunately this code was prematurely released GA when it was migrated as a top level module a short time later in #7908 without any feature flags. This has now lead to users reporting memory leaks along with several other bugs in the zstd jni library. Additionally, Lucene has a long running discussion on the pros/cons of Zstd as an index compression option including the reasons it's not provided as a core capability. One of those reasons is the hard dependency on native compiled code, which often leads to portability issues due to glibc differences. These issues have been realized several times both in the OpenSearch bundle (e.g., see the KNN issue where users can't compile on M1 Mac) and legacy codebase .
For these reasons, we need to address the premature promotion of the zstd library as a GA / LTS feature in core, and (at minimum) release a patched bundle that fixes the critical performance issues and bugs.
Describe the solution you'd like
Quickly build and release a 2.9.1 bundle distribution with five patches.
ZSTD_COMPRESSION_EXPERIMENTAL = "opensearch.experimental.feature.compression.zstd.enabled
feature flag that is set tofalse
by default (forcing users to opt in).ZSTD_COMPRESSION_EXPERIMENTAL
feature flag both to the CodecService constructor and theCompressionProvider.getCompressors()
(used for BlobStoreRepository compression).DeprecationLogger
message that the zstd feature will be moved to a plugin in the next release1.5.5-3
to1.5.5-5
Bump zstd version to 1.5.5-5 #9431 (NEEDS TO BE BACKPORTED)In 2.10.0 (or later even) we should decide the following:
ZstdCodec
andZstdNoDictCodec
out from being a default module into an optional location (e.g., either an optional plugin or library - note that the BlobStoreRepository compression is already in as an optional library, but its packaged and included by default so we still need to figure out how to make that optional).Describe alternatives you've considered
The text was updated successfully, but these errors were encountered: