Skip to content
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

Feature flag for ZStandard #9476

Conversation

sarthakaggarwal97
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 commented Aug 22, 2023

Signed-off-by: Sarthak Aggarwal [email protected]

Description

This PR is to feature flag ZStd compression codecs

Related Issues

Resolves #9422

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

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the zstd-feature-flag branch 2 times, most recently from 34273a1 to 15231ef Compare August 22, 2023 09:37
@sarthakaggarwal97 sarthakaggarwal97 changed the title feature flagging of Zstandard Feature flag for Zstandard Aug 22, 2023
@sarthakaggarwal97 sarthakaggarwal97 changed the title Feature flag for Zstandard Feature flag for ZStandard Aug 22, 2023
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the zstd-feature-flag branch 3 times, most recently from 7cea476 to 1771d1d Compare August 22, 2023 09:40
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change dd75a22

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change dd75a22

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change dd75a22

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #9476 (6e2e2f6) into main (ebdffbb) will decrease coverage by 0.12%.
Report is 5 commits behind head on main.
The diff coverage is 23.46%.

@@             Coverage Diff              @@
##               main    #9476      +/-   ##
============================================
- Coverage     71.20%   71.08%   -0.12%     
+ Complexity    57474    57436      -38     
============================================
  Files          4776     4778       +2     
  Lines        270815   270900      +85     
  Branches      39584    39589       +5     
============================================
- Hits         192835   192581     -254     
- Misses        61741    62137     +396     
+ Partials      16239    16182      -57     
Files Changed Coverage Δ
...earch/script/expression/ExpressionScoreScript.java 0.00% <0.00%> (ø)
...arch/script/expression/ExpressionScriptEngine.java 33.99% <ø> (ø)
...earch/example/expertscript/ExpertScriptPlugin.java 0.00% <0.00%> (ø)
...pensearch/common/settings/FeatureFlagSettings.java 50.00% <ø> (ø)
...ry/functionscore/TermFrequencyFunctionFactory.java 0.00% <0.00%> (ø)
...n/java/org/opensearch/script/ScoreScriptUtils.java 1.43% <0.00%> (-0.31%) ⬇️
...nsearch/search/lookup/LeafTermFrequencyLookup.java 35.71% <35.71%> (ø)
...c/main/java/org/opensearch/script/ScoreScript.java 56.92% <50.00%> (-1.15%) ⬇️
...java/org/opensearch/index/engine/EngineConfig.java 94.90% <60.00%> (-1.15%) ⬇️
...nsearch/painless/action/PainlessExecuteAction.java 77.58% <100.00%> (+0.29%) ⬆️
... and 4 more

... and 476 files with indirect coverage changes

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change dd75a22

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change dd75a22

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.client.PitIT.testDeleteAllAndListAllPits

@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review August 22, 2023 13:23
@sarthakaggarwal97
Copy link
Contributor Author

sarthakaggarwal97 commented Aug 23, 2023

@nknize @andrross please check this out, I've addressed the comments, build is green.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 5d3633c

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 5d3633c

Incompatible components

Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@andrross
Copy link
Member

The base integ test class will randomly select a codec from this list:

public static final List<String> CODECS = List.of(
CodecService.DEFAULT_CODEC,
CodecService.LZ4,
CodecService.BEST_COMPRESSION_CODEC,
CodecService.ZLIB,
CodecService.ZSTD_CODEC,
CodecService.ZSTD_NO_DICT_CODEC

If ZSTD is in there, then the base class must enable the feature flag or else you will randomly hit failures. @nknize what do you think? Should we allow ZSTD to be randomly selected in any integ test?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this change correctly, this is putting back a feature that was shipped GA in 2.9 behind a feature flag, which is effectively a breaking change for 3.0. Assuming this wants to be backported to 2.x, it will be a violation of semver There's a strong support in #9422 (comment) from at least 6 people, including multiple maintainers, against this change.

@sarthakaggarwal97
Copy link
Contributor Author

sarthakaggarwal97 commented Aug 24, 2023

@dblock your understanding is correct. I just kept changes ready, and will proceed inline with what the community has decided. Thank you!

@sarthakaggarwal97
Copy link
Contributor Author

Closing the PR in lieu of the community's decision at #9422

@nknize nknize reopened this Aug 29, 2023
@nknize nknize requested a review from msfroh as a code owner August 29, 2023 03:04
@nknize
Copy link
Collaborator

nknize commented Aug 29, 2023

Re-opening as there is no community decision on this as of yet.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

sarthakaggarwal97 and others added 2 commits August 29, 2023 10:02
Signed-off-by: Sarthak Aggarwal <[email protected]>
ZSTD can be randomly selected by the integ tests as an index codec so
the feature flag must be enabled by default.

Signed-off-by: Andrew Ross <[email protected]>
@andrross
Copy link
Member

I rebased and added a fix for the integration tests just to get this ready while we work towards a community decision

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@andrross
Copy link
Member

Closing as we have completed the move of the ZSTD codec to a plugin repository

@andrross andrross closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Demote] ZStd compression from GA / LTS to experimental and release a 2.9.1 patch
5 participants