-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix toBuilder method in EngineConfig to include mergedSegmentTransfer… #20105
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
WalkthroughThe pull request adds support for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…Tracker Signed-off-by: Uday Bhaskar <[email protected]>
970f63f to
65327b2
Compare
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/index/engine/EngineConfig.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
server/src/main/java/org/opensearch/index/engine/EngineConfig.java (2)
357-358: Excellent fix for the toBuilder() completeness.The addition of both
clusterApplierServiceandmergedSegmentTransferTrackerto thetoBuilder()method correctly addresses the NPE issue described in the PR. Both fields were missing from the builder chain, which would causegetMergedSegmentTransferTracker()andgetClusterApplierService()to return null on configs created viatoBuilder().The fix ensures that plugins like opensearch-storage-encryption that rely on the builder pattern will receive fully populated configuration objects.
826-829: Builder method follows the established pattern.The
mergedSegmentTransferTracker()builder method correctly follows the standard builder pattern used throughout this class, enabling method chaining and proper field assignment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20105 +/- ##
============================================
- Coverage 73.33% 73.33% -0.01%
- Complexity 71679 71708 +29
============================================
Files 5790 5790
Lines 327549 327666 +117
Branches 47181 47205 +24
============================================
+ Hits 240217 240279 +62
- Misses 68080 68123 +43
- Partials 19252 19264 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…Tracker (opensearch-project#20105) Signed-off-by: Uday Bhaskar <[email protected]> Co-authored-by: Uday Bhaskar <[email protected]>
…Tracker (opensearch-project#20105) Signed-off-by: Uday Bhaskar <[email protected]> Co-authored-by: Uday Bhaskar <[email protected]>
…Tracker (opensearch-project#20105) Signed-off-by: Uday Bhaskar <[email protected]> Co-authored-by: Uday Bhaskar <[email protected]>
Description
Recently a change was introduced to Include
MergedSegmentTransferTrackerin EngineConfig, but the builder method was not updated, leading to consumers of builder method returningnullwhen getMergedSegmentTransferTracker() is invoked. This is leading to_cat/indicesapi not able to fill details properly.Currently the builder method is used by
opensearch-storage-encryptionplugin.Error earlier in
_cat/indicescall -Response
_cat/indicesAfter adding missing
MergedSegmentTransferTracker,_cat/indicesworking as expectedRelated Issues
Resolves #[Issue number to be closed when this PR is merged]
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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.