-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add toBuilder() method in EngineConfig #19054
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
Conversation
|
❌ Gradle check result for 522d865: FAILURE 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? |
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
522d865 to
ce9558c
Compare
|
❌ Gradle check result for ce9558c: FAILURE 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? |
server/src/main/java/org/opensearch/index/codec/CodecService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/engine/EngineConfig.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Rajat Gupta <gptrajat@amazon.com>
cwperks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @RajatGupta02 ! The changes look good to me. For any other maintainer reviewing this PR, you can see the usage here: https://github.com/opensearch-project/opensearch-storage-encryption/pull/39/files#diff-e1145e4d7e594b9d06cdc9d1a8da84d0756b05bbab95c57174ecfa82ada15facR47-R52
The responsibility for ensuring that the builder has all necessary values would fall on the EngineConfig constructor that accepts the builder so I think the changes in here are reasonable.
|
❌ Gradle check result for 707c7cf: FAILURE 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? |
|
@RajatGupta02 i think we should also point out the problem we were facing in the encryption plugin which motivated us to make this change. |
|
❌ Gradle check result for 707c7cf: FAILURE 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? |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19054 +/- ##
============================================
- Coverage 73.02% 72.97% -0.05%
+ Complexity 69548 69478 -70
============================================
Files 5647 5647
Lines 319106 319136 +30
Branches 46163 46163
============================================
- Hits 233034 232900 -134
- Misses 67232 67423 +191
+ Partials 18840 18813 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Add a toBuilder() method Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Fix release version Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
* Add a toBuilder() method Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Fix release version Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
* Add a toBuilder() method Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Fix release version Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
* Add a toBuilder() method Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Fix release version Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
* Add a toBuilder() method Signed-off-by: Rajat Gupta <gptrajat@amazon.com> * Fix release version Signed-off-by: Rajat Gupta <gptrajat@amazon.com> --------- Signed-off-by: Rajat Gupta <gptrajat@amazon.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Rajat Gupta <gptrajat@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
Description
Adding a toBuilder() method for easy modification of specific fields while preserving all other configs.
Motivation
For our opensearch storage encryption plugin, while implementing translog encryption, in our CryptoEngineFactory we needed to create an EngineConfig identical to the original except for the translogFactory (to enable encrypted translogs).
This would mean copying all fields like below, and we would miss the configs if the original configs changed in Builder :
Hence, with this PR, it enables selective modification of specific fields while preserving others.
Related Issues
Resolves
Check List
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.