Fix: avoid ZSTD codec from overriding service codec factory.

[mulugetam]

The new ZSTD compression codec adds ZSTD to the existing compression codecs (default, best_compression, and lucene_default). This PR allows the compression codec to give a custom-codec service factory only when index.codec is set to ZSTD or ZSTDNODICT.

Description

Fixes issue #7012 by explicitly avoiding the creation of a custom codec service unless the index.codec value is either ZSTD or ZSTDNODICT .

Issues Resolved

Resolves #7012

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• [x ] Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[dblock]

This works, but feels awfully specific to the fact that we have these two custom codecs in the project. A few ideas before I hit approve/merge.

1. Can we check whether the codec is not custom (aka well known codecs) instead of whether it's a known custom codec?
2. Does it make sense to make names such as "CUSTOM:ZSTD" and then look for "CUSTOM:" instead or is it a silly idea?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/13665/
• CommitID: bc037e2
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[navneet1v]

please add unit test for this class.

[mulugetam]

1. dec is not custom

@dblock Do you mean requiring the user to specify all custom codecs as, for example, index.codec: CUSTOM:ZSTD instead of just index.codec: ZSTD? And then return a custom CodecServiceFactory only if(codec.startsWith('CUSTOM:'))?

[navneet1v]

One comment, if an index sets this value to use the custom codec and from another plugin the codec is coming lets say a k-NN index, which codec will be picked up? or it will lead to failures?

Example(might result in failure, can we check this case):

PUT my-knn-index-1
{
  "settings": {
    "index": {
      "knn": true,
      "knn.algo_param.ef_search": 100,
      "codec": "ZSTD"
    }
  },
  "mappings": {
    "properties": {
        "my_vector1": {
          "type": "knn_vector",
          "dimension": 2,
          "method": {
            "name": "hnsw",
            "space_type": "l2",
            "engine": "nmslib",
            "parameters": {
              "ef_construction": 128,
              "m": 24
            }
          }
        },
        "my_vector2": {
          "type": "knn_vector",
          "dimension": 4,
          "method": {
            "name": "hnsw",
            "space_type": "innerproduct",
            "engine": "faiss",
            "parameters": {
              "ef_construction": 256,
              "m": 48
            }
          }
        }
    }
  }
}

I have created this issue: #7032 tries to explore the possible solution.

One suggestion I have is can we add java doc on top of this plugins and also on the EnginePlugin class which provides this interface that if an index tries to use 2 codec this can lead to failures in creating the index.

[mulugetam]

I reviewed the code again to see if I could do it without overriding the existing code service factory, and I can. So, I have removed the CodecService classes altogether.

Note that the custom compression codecs are registered by calling org.apache.lucene.codecs.Codec ctor here. The index.codec setting will get the custom compression codecs by calling Codec.availableCodecs().contains(s) here in EngineConfig.

@dblock The names for custom compression codecs, ZSTD and ZSTDNODICT, cannot contain non-alphanumeric characters; Lucene's NamedSPILoader forbids it.

@navneet1v @martin-gaievski Can you check if this also addresses #7032?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/13682/
• CommitID: a76ed95

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.72%. Comparing base (53b128f) to head (0cf7be4).
⚠️ Report is 4025 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7037      +/-   ##
============================================
- Coverage     70.78%   70.72%   -0.07%     
- Complexity    59269    59278       +9     
============================================
  Files          4823     4820       -3     
  Lines        283985   283962      -23     
  Branches      40953    40952       -1     
============================================
- Hits         201026   200820     -206     
- Misses        66403    66693     +290     
+ Partials      16556    16449     -107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[navneet1v]

I reviewed the code again to see if I could do it without overriding the existing code service factory, and I can. So, I have removed the CodecService classes altogether.

@mulugetam

Now you have removed all CodecSeviceFactory , I want to know how the codec is now getting used or getting attached to a particular index?

is it like if someone specify the index.codec: ZSTD the ZstdCodec would be picked up if already present?

[mulugetam]

is it like if someone specify the index.codec: ZSTD the ZstdCodec would be picked up if already present?

@navneet1v Yes. That happens here, which calls Lucene's registered codecs here.

Lucene's setCodec(new ZstdCodec()) is called, just like setCodec(new Lucene95Codec(Lucene95Codec.Mode.BEST_COMPRESSION)) is called for a index.codec: best_compression.

Flamegraphs for the run below, with default, best_compression, and ZSTD compression codecs, are available here (search for "compress").

opensearch-benchmark execute_test \
--workload=geonames \
--workload-params='{"index_settings":{"index.codec": "compression_codec"}}' \
--target-hosts=127.0.0.1:9200 \
--kill-running-processes \
--pipeline=benchmark-only \
--include-tasks=delete-index,create-index,index-append \
--kill-running-processes

[reta]

I reviewed the code again to see if I could do it without overriding the existing code service factory, and I can. So, I have removed the CodecService classes altogether.

This is awesome, @mulugetam , basically the standard service loader mechanism is purely sufficient here, right?

[mulugetam]

This is awesome, @mulugetam , basically the standard service loader mechanism is purely sufficient here, right?
@reta yes.

[mulugetam]

@reta I have also upgraded the zstd-jni version from 1.5.4-1 to 1.5.5-1. Version 1.5.5-1 is based on ZSTD version 1.5.5 that addresses the rare corruption bug described here: https://github.com/facebook/zstd/releases/tag/v1.5.5

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica
      1 org.opensearch.indices.replication.SegmentReplicationIT.testReplicaHasDiffFilesThanPrimary
      1 org.opensearch.indices.replication.SegmentReplicationIT.testPitCreatedOnReplica

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