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

Accessors for custom compression modes #89

Merged

Conversation

sarthakaggarwal97
Copy link
Collaborator

Description

Enables accessors for custom compression modes.

Issues Resolved

#88

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.

@sarthakaggarwal97 sarthakaggarwal97 changed the title accessors for custom compression modes Accessors for custom compression modes Nov 30, 2023
@sarthakaggarwal97 sarthakaggarwal97 self-assigned this Nov 30, 2023
@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review November 30, 2023 08:17
@sarthakaggarwal97
Copy link
Collaborator Author

@reta please take a look! Thank you :D
I've added a test for the getters, however lmk if I should remove it. I think, in general, we don't need to test the getters/accessors.

@reta
Copy link
Collaborator

reta commented Nov 30, 2023

I've added a test for the getters, however lmk if I should remove it. I think, in general, we don't need to test the getters/accessors.

@sarthakaggarwal97 sure, what is the practical reason of exposing these properties?

@sarthakaggarwal97
Copy link
Collaborator Author

@reta I'm working on a plugin which requires me to have my own implementation of the Lucene90CompressingStoredFieldsFormat to optimize on read operations.
With this implementation, I would like to add support for ZstdCompressionMode and ZstdNoDictCompressionMode which requires me to pass them as arguments, and custom-codecs package would be able to provide the compression modes out of the box while avoiding any code/implementation duplication.
I don't see any risks in exposing the compression modes via getters. Please lmk if you see any concerns.

@reta
Copy link
Collaborator

reta commented Dec 5, 2023

I don't see any risks in exposing the compression modes via getters. Please lmk if you see any concerns.

@sarthakaggarwal97 There are few issues here:

  • this plugin does not publish any artifacts (at least not supposed to) besides ZIP distribution, so the way this API is consumed in unclear to me
  • the exposed API should be something like
    public CompressionMode getCompressionMode() {
        return mode == ZSTD_NO_DICT ? zstdNoDictCompressionMode : zstdCompressionMode;
    }
    

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the accessors-compression-mode branch 2 times, most recently from 7cb4065 to d77955a Compare December 13, 2023 10:32
@sarthakaggarwal97
Copy link
Collaborator Author

@reta yes, if we want to just fetch the compression mode, we should use the API you suggested.
I've updated the PR to now return a map of compression modes against the respective Mode.

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the accessors-compression-mode branch 2 times, most recently from 1b5d4f3 to 403f3af Compare December 14, 2023 06:44
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@reta reta merged commit 804fe11 into opensearch-project:main Dec 15, 2023
12 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 15, 2023
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
(cherry picked from commit 804fe11)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Dec 15, 2023
(cherry picked from commit 804fe11)

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants