Add adaptive merge policy to reduce benchmark variance #19352
Conversation
rgsriram
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
github-actions
bot
added
enhancement
|
❌ Gradle check result for 2b398ff: 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? |
rgsriram
force-pushed
the
issue-11163
branch
from
2b398ff to
e88f69b
Compare
|
❌ Gradle check result for abe1e49: 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? |
rgsriram
force-pushed
the
issue-11163
branch
from
abe1e49 to
5076ee5
Compare
|
❌ Gradle check result for 5076ee5: 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? |
rgsriram
force-pushed
the
issue-11163
branch
from
5076ee5 to
6f0df5f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java (1)
13-85: Consider adding integration and edge-case tests.The current tests verify data class construction and field access, which is a good foundation. However, the test suite would benefit from:
- Integration tests that exercise
SegmentTopologyAnalyzer's analysis logic with realistic segment distributions- Edge cases such as empty segments, single segment, or extreme size variations
- Scenarios that validate the recommendation generation logic
These additions would provide stronger confidence in the adaptive merge policy behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
🔇 Additional comments (3)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java (3)
15-30: LGTM!The test correctly verifies all fields of the
Segmentdata class.
32-59: LGTM!All
SegmentMetricsfields are now properly asserted, including the previously missingmedianSizeBytes,meanSizeBytes,variance, andskew. The coefficient-of-variation calculation is verified with appropriate tolerance.
61-84: LGTM!The test correctly verifies all fields of the
MergePolicyRecommendationsclass, including boolean flags and numeric thresholds. The descriptive assertion messages are helpful.
|
❌ Gradle check result for c244676: 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? |
|
❌ Gradle check result for 5fa91cd: 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? |
rgsriram
force-pushed
the
issue-11163
branch
from
5fa91cd to
d9480b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (1)
91-97: Test validates boundary values that don't align with smooth interpolation.
testMergePolicySettingsValidationuses discrete boundary values (50MB, 200MB, 1GB, 5GB for max segment) that represent the interpolation endpoints, not intermediate values. The currentvalidateMergeSettingshelper only performs range checks (positive, <= 5GB, floor < max, segments 5-20), so this works, but it doesn't actually validate that the production calculator produces these exact values for the given categories.Consider either:
- Adding tests that call
SegmentTopologyTestUtils.getRecommendedMaxSegmentSize()with specific shard sizes and assert exact outputs, or- Renaming/documenting this test to clarify it validates theoretical category boundaries, not production calculator outputs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdserver/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.javaserver/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.javaserver/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.javaserver/src/main/java/org/opensearch/index/IndexSettings.javaserver/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/rest/action/admin/package-info.javaserver/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.javaserver/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.javaserver/src/test/java/org/opensearch/index/analysis/SimpleSegmentTopologyTests.javaserver/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java
🚧 Files skipped from review as they are similar to previous changes (4)
- server/src/main/java/org/opensearch/rest/action/admin/package-info.java
- server/src/test/java/org/opensearch/index/analysis/SimpleSegmentTopologyTests.java
- server/src/main/java/org/opensearch/index/IndexSettings.java
- server/src/main/java/org/opensearch/index/shard/IndexShard.java
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
server/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.java (1)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
server/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.java (1)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: detect-breaking-change
🔇 Additional comments (17)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
24-122: Well-structured utility class with sound interpolation logic.The centralization of adaptive merge policy calculations into this utility class is a good design choice, avoiding duplication across
AdaptiveOpenSearchTieredMergePolicy,SegmentTopologyAnalyzer, and test utilities. The piecewise linear interpolation across log10 ranges provides smooth transitions without abrupt parameter jumps.Minor observation: The
/ 1.0in ratio calculations (e.g., line 45, 50) is mathematically redundant since the numerator is already adouble, but it's harmless and arguably improves readability by making the interpolation formula explicit.server/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.java (3)
70-84: LGTM! Clean adaptive policy design.The dual-policy architecture (regularMergePolicy for normal merges, forcedMergePolicy with unlimited max for force-merges) correctly mirrors the OpenSearch merge behavior. The settingsLock injection enables proper synchronization with external callers.
86-164: Solid implementation of adaptive merge behavior with proper error handling.The segment size aggregation with failure tracking and tiered logging is well implemented. The write lock correctly protects only the settings update (lines 153-160), not the subsequent
findMerges()delegation, which is the right approach - the merge planning can proceed with the just-updated settings without holding the lock.
181-297: Properly synchronized getters and setters.All setter/getter pairs consistently use the read/write lock pattern, addressing the prior review feedback about thread-safety. The
setMaxMergedSegmentMBcorrectly excludes forcedMergePolicy (line 238-246 comment) since forced merges should remain unlimited.server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.java (1)
30-69: Clean provider implementation.The provider correctly initializes the adaptive policy with sensible defaults and properly delegates the
INDEX_MERGE_ENABLEDsetting handling. ThesettingsLockis shared with the policy, enabling synchronized updates from both the provider and the policy'sfindMerges()path.Note: The default
floorSegmentMBof 16 MB differs from Lucene's default of 2 MB. This is intentional per the PR objectives to produce more balanced segment topologies.CHANGELOG.md (1)
13-13: LGTM!The changelog entry is correctly formatted, placed in the appropriate "Added" section, and references the linked issue #11163.
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
19-53: LGTM! Correct delegation to production calculator.The test utilities now properly delegate to
AdaptiveMergePolicyCalculator, ensuring test behavior exactly mirrors production. This addresses the previous review concern about divergence between discrete tier-based test values and smooth production interpolation.server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (2)
50-71: Placeholder test is appropriately documented.The
testPerformanceComparisonPlaceholdermethod clearly documents that it uses hardcoded values and serves as a placeholder for future real benchmarks. The TODO comments make the intent clear.
192-206: LGTM! Variance calculation now uses double arithmetic.The variance calculation correctly uses
doublethroughout (lines 196-201), addressing the previous overflow concern for large segment sizes.server/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java (3)
44-71: Good coverage of SegmentMetrics including derived fields.The test validates all 9 constructor parameters plus the calculated
coefficientOfVariation, addressing the previous review concern about incomplete field assertions.
190-217: Mock setup correctly simulates segment size.The
createSegmentCommitInfohelper properly mocksDirectory.fileLength()to return the specified size for the segment's compound file (.cfe), which aligns with howSegmentCommitInfo.sizeInBytes()aggregates file sizes.
169-188: Verify skew threshold assertion matches production logic.Line 183 asserts
metrics.skew > 3000which correctly accounts for the* 1000scaling used inSegmentTopologyAnalyzer.calculateMetrics(). However, line 214 inSegmentTopologyAnalyzer.analyzeAndRecommend()usesmetrics.skew > 3000as the threshold, so this test is validating the correct behavior.server/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.java (5)
40-52: Public API surface is well-defined.The
@PublicApi(since = "3.3.0")annotation and clear class/method documentation establish this as a stable API for analyzing segment topology. The immutablesegmentslist (line 127, 143) ensures internal invariants are preserved.
158-210: Metrics calculation is mathematically correct.The
calculateMetrics()implementation properly handles:
- Median for even-sized lists (average of two middle values, lines 179-181)
- Variance using double arithmetic to avoid overflow (lines 189-194)
- Skew stored with × 1000 precision (line 208)
The coefficient of variation is derived correctly as
sqrt(variance) / meanSizeBytes(line 346).
212-235: Recommendations correctly use centralized calculator.
analyzeAndRecommend()properly delegates toAdaptiveMergePolicyCalculatorfor smooth interpolation-based recommendations. The skew threshold (line 214) correctly uses> 3000to match the × 1000 scaled storage, andsizeSkewis exposed unscaled (line 230) as previously reviewed.
269-301: Segment inner class is correctly serializable.The
Segmentclass properly implementsWriteablewith matchingStreamInputconstructor andwriteTo()method. The field order is consistent between serialization and deserialization.
309-348: SegmentMetrics exposes both raw and derived statistics.The class correctly exposes both the raw
variance(bytes²) and the derivedcoefficientOfVariation(dimensionless). Theskewfield's × 1000 scaling is documented in the comment (line 319).
|
❌ Gradle check result for d9480b6: 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? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (4)
1-25: LGTM - Clean test class structure.The package, imports, and class declaration are well-organized. The javadoc clearly describes the purpose of the benchmark tests.
31-78: LGTM - Well-structured validation tests.The first three test methods provide good coverage:
- Validates recommendations across shard size ranges
- Compares topology metrics (variance, balance)
- Includes clear documentation about placeholder status for performance comparison
106-185: LGTM - Helper methods are well-implemented.The validation and simulation helpers are straightforward:
validateRecommendationsForShardSizeperforms reasonable boundary checks- Topology simulation methods create realistic test scenarios
- Placeholder methods are clearly documented with TODOs
- Categorization logic correctly handles boundary conditions
226-284: LGTM - Metrics calculation and data structures are sound.The implementation correctly handles numerical calculations:
- Variance uses double arithmetic to avoid overflow (previously fixed)
- Balance score provides a normalized [0,1] metric
- Inner classes cleanly encapsulate metrics
|
❌ Gradle check result for 95ac08c: null 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? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (3)
9-9: Consider relocating to a merge-related package.The test class is in
org.opensearch.index.analysisbut validates merge policy behavior and segment topology—neither of which relates to text analysis. A package likeorg.opensearch.index.mergewould more accurately reflect the test's purpose and make it easier for future contributors to locate merge-related tests.
120-134: LGTM with optional refinement: Consider using calculator for max segment bound.The sanity checks are appropriate for this test. However, line 127 compares against a hardcoded
5L * 1024 * 1024 * 1024constant. If you want to ensure this bound stays synchronized with the calculator's actual maximum, consider fetching the max from the production calculator for the given shard size or referencing a shared constant.
189-199: Optional: Document the categorization boundaries.The thresholds (100MB, 1GB, 10GB) are hardcoded without explanation. While the test correctly validates these boundaries, a brief comment explaining their rationale would help future maintainers understand whether these are aligned with production calculator ranges or chosen for test coverage purposes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyBenchmarkTests.java (3)
42-50: LGTM: Variance and balance calculations are numerically sound.The segment topology simulation and metric calculations correctly validate that balanced topologies have lower variance and better balance scores. The variance overflow issue noted in previous reviews has been properly addressed by using
doublefor accumulation.Also applies to: 136-153, 232-256
52-78: LGTM: Placeholder status is now clearly documented.The previous concern about hardcoded values has been addressed. The method names, inline comments, and TODO annotations make it clear this test awaits integration with actual benchmark measurements. The directional assertions provide a basic structural check until real performance data becomes available.
Also applies to: 155-187
258-290: LGTM: Test data structures are well-designed.The private inner classes and enum appropriately encapsulate test-specific metrics and categories. The
SegmentTopologyMetrics.variancefield correctly usesdoubleto avoid the overflow issue addressed in previous reviews.
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyBenchmarkTests.java (1)
267-292: Consider removing or utilizing unused fields.The following fields are initialized but never accessed:
SegmentTopologyMetrics.segmentCountSegmentTopologyMetrics.totalSizePerformanceMetrics.avgLatencyIf these are intentionally reserved for future enhancements or debugging visibility, consider adding a brief comment. Otherwise, removing them would reduce noise.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
🔇 Additional comments (4)
server/src/test/java/org/opensearch/index/merge/SegmentTopologyBenchmarkTests.java (4)
20-25: LGTM! Previous review issue addressed.The
@opensearch.apitag was correctly removed from the test class Javadoc.
241-255: LGTM! Variance overflow issue addressed.The variance calculation now correctly uses
doubleto prevent overflow when computing squared differences for large segment sizes (e.g., 5GB segments).
208-239: LGTM! Regression protection for max segment size now included.The
validateMergeSettingsmethod now includesexpectedMaxSegmentparameter and validates it against the calculated value, providing complete regression protection for all three merge policy parameters.
31-37: Approve the well-structured test coverage.The tests provide good coverage across shard size categories (50MB to 50GB) and correctly test boundary conditions for categorization. The combination of sanity checks and exact-value regression tests in
testMergePolicySettingsValidationis a solid approach for catching unintended changes to the adaptive policy calculations.Also applies to: 88-97
|
❕ Gradle check result for 7396c46: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (2)
67-85: Remove orphaned placeholder javadoc block.There are two consecutive javadoc blocks before
testPerformanceComparison. The first block (lines 67-76) describes a "placeholder test" with "hardcoded placeholder values," while the second block (lines 77-85) accurately describes the actual simulation. Only the second javadoc applies to the method; the first is orphaned and contradicts the implementation.🔎 Proposed fix
- /** - * Placeholder test for performance comparison between defaults and adaptive - * settings. - * - * NOTE: This test currently uses hardcoded placeholder values and does not - * validate - * actual production behavior. It serves as a placeholder for future real - * benchmark - * integration. Real benchmarks should measure actual merge performance metrics. - */ /** * Test for performance comparison between defaults and adaptive settings. * <p>
228-230: Consider using an explicitMergeTriggerinstead of null.Passing
nullfor theMergeTriggerparameter works if the policy doesn't inspect it, but using an explicit value likeMergeTrigger.SEGMENT_FLUSHwould be more robust and self-documenting.🔎 Proposed fix
+import org.apache.lucene.index.MergeTrigger;- MergeSpecification spec = mergePolicy.findMerges(null, segmentInfos, mergeContext); + MergeSpecification spec = mergePolicy.findMerges(MergeTrigger.SEGMENT_FLUSH, segmentInfos, mergeContext);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
🔇 Additional comments (8)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (8)
86-116: LGTM!The test appropriately validates that both policies produce sane metrics without asserting one is strictly better than the other, which is the right approach given the inherent randomness in the simulation. Logging results for visibility is helpful for debugging.
246-262: LGTM!The merge application logic correctly removes merged segments and adds the combined segment. While
List.remove(Object)removes only the first matching occurrence (which could theoretically misfire with duplicate sizes), the random noise in segment sizing makes exact duplicates unlikely in this simulation.
264-291: LGTM!The mocking approach for
SegmentCommitInfois appropriate for unit testing. Using.cfesuffix aligns with Lucene's compound file format conventions.
305-336: LGTM!The regression protection for max segment size has been properly added as requested in the previous review. The method now validates all three merge policy settings with both sanity checks and exact value assertions.
338-352: LGTM!The variance overflow issue has been properly addressed by using
doublefor the variance calculation. The division on line 340 is safe since all callers ensure non-empty segment lists.
35-40: LGTM!The
@opensearch.apitag has been properly removed from the test class javadoc as requested in the previous review.
46-65: LGTM!The recommendation validation and topology variance impact tests are well-structured with clear test scenarios covering different shard sizes and topology configurations.
141-161: LGTM!The regression test values are properly documented with comments explaining the expected calculations. Including the category classification, shard size, and all three expected values provides comprehensive regression protection.
|
❌ Gradle check result for b85e386: 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? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
214-214: Minor typo: "Flashing" should be "Flushing".🔎 Suggested fix
- // 1. Simulate Flashing a new segment + // 1. Simulate flushing a new segment
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: check-files
🔇 Additional comments (4)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (4)
36-41: LGTM!The class structure is clean and the
@opensearch.apitag has been appropriately removed from the test class as noted in the previous review.
78-110: Test design is appropriate for non-deterministic simulation.The weak assertions (checking only
>= 0for variance and> 0for consistency) are intentional and correctly documented. Given the random noise in the simulation, comparing absolute values between policies would lead to flaky tests. Logging results for visibility is a good practice here.
332-346: Variance calculation usingdoubleis correct.The previous overflow issue has been properly addressed by using
doublefor variance accumulation and storage.
299-330: Regression protection for max segment size is now in place.The
expectedMaxSegmentparameter and corresponding assertion (line 322) have been added as suggested in the previous review.
Signed-off-by: Sriram Ganesh <[email protected]> Refactored the code Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments Signed-off-by: Sriram Ganesh <[email protected]> Refactored the code Signed-off-by: Sriram Ganesh <[email protected]> Fixed style checks Signed-off-by: Sriram Ganesh <[email protected]> Addressed nitpicks Signed-off-by: Sriram Ganesh <[email protected]> Addressed comments fron code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the nitpick from code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the nitpick from code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments from code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments from code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the comments from code rabbit Signed-off-by: Sriram Ganesh <[email protected]> Addressed the nitpick from code rabbit Signed-off-by: Sriram Ganesh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.java (1)
32-69: Behavior and defaults look consistent; consider explicitly documenting parity with tiered defaults.The provider correctly:
- Shares a
ReentrantReadWriteLockwithAdaptiveOpenSearchTieredMergePolicy.- Respects
INDEX_MERGE_ENABLEDby falling back toNoMergePolicy.- Seeds the adaptive policy with sensible defaults (5GB max, 16MB floor, 10 segments per tier, etc.).
If you haven’t already, it’s worth double‑checking these values match the existing
TieredMergePolicyProviderdefaults so switching toadaptivedoesn’t unexpectedly change non‑adaptive behavior, and then calling that out briefly in docs or comments.server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
78-110: Optional: tightentestPerformanceComparisonandtestSegmentTopologyVarianceImpactassertions with seed control if stronger regression protection is desired.OpenSearch's test framework uses Carrotsearch's RandomizedTest, which supports deterministic seeding to ensure reproducible test runs. You can use either:
@Seed("MASTER:TEST")annotation on the test method- System property
-Drt.seed=MASTER:TESTat runtimeThis allows you to fix the random sequence and then assert relative improvements (e.g., adaptive variance ≤ default variance, or higher consistency score) on the same simulated workload, providing stronger regression protection while still controlling flakiness.
The current weak assertions (non-negative variance, positive consistency score) are fine for avoiding false failures; stronger assertions are optional for future hardening.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdserver/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.javaserver/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.javaserver/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.javaserver/src/main/java/org/opensearch/index/IndexSettings.javaserver/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/rest/action/admin/package-info.javaserver/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.javaserver/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.javaserver/src/test/java/org/opensearch/index/analysis/SimpleSegmentTopologyTests.javaserver/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java
🚧 Files skipped from review as they are similar to previous changes (5)
- server/src/test/java/org/opensearch/index/analysis/SimpleSegmentTopologyTests.java
- server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java
- server/src/main/java/org/opensearch/rest/action/admin/package-info.java
- server/src/test/java/org/opensearch/index/merge/SegmentTopologyAnalyzerTests.java
- server/src/main/java/org/opensearch/index/shard/IndexShard.java
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
server/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.java (2)
server/src/main/java/org/opensearch/index/IndexSettings.java (1)
PublicApi(96-2323)server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
server/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.java (1)
server/src/main/java/org/opensearch/index/AdaptiveMergePolicyCalculator.java (1)
AdaptiveMergePolicyCalculator(24-122)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (22)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: Mend Security Check
🔇 Additional comments (5)
CHANGELOG.md (1)
13-13: Changelog entry looks correct and well-scoped.The new “adaptive merge policy” bullet is clear, references the right issue, and fits the existing Unreleased 3.x Added section with no duplication or formatting issues.
server/src/main/java/org/opensearch/index/AdaptiveOpenSearchTieredMergePolicy.java (1)
86-164: Adaptive policy topology recomputation and locking look solid.Computing
totalSizeBytesfromSegmentInfoswith explicit handling and logging forIOException, deriving settings viaAdaptiveMergePolicyCalculator, and applying them under the sharedsettingsLockbefore delegating to the innerTieredMergePolicygives you a race‑free adaptive behavior. The corresponding getters/setters consistently use the same read/write lock, so external configuration changes and merge‑time recalculations won’t step on each other.Also applies to: 181-297
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
21-52: Good consolidation: test helpers now mirror production calculator exactly.Delegating all three helpers to
AdaptiveMergePolicyCalculatorremoves duplicated math and keeps test expectations aligned with production logic across all shard sizes.server/src/main/java/org/opensearch/index/IndexSettings.java (1)
101-133: Adaptive merge policy is cleanly integrated into IndexSettings.Adding the
ADAPTIVEenum value, instantiatingAdaptiveTieredMergePolicyProvideralongside existing providers, and routing both index‑level and time‑series defaults through it whenadaptiveis selected keeps behavior explicit while avoiding public exposure of the provider itself.Also applies to: 929-930, 1172-1175, 1973-2001, 2322-2323
server/src/main/java/org/opensearch/index/merge/SegmentTopologyAnalyzer.java (1)
53-235: Segment metrics and recommendation API now look correct and internally consistent.The analyzer:
- Safely builds an unmodifiable segment snapshot with clear logging for partial/failed sizing.
- Computes min/max/median/mean, variance, and a simple skew metric without overflow, with a proper even‑sized median.
- Uses coefficient of variation and appropriately scaled skew thresholds for flags, while returning true variance and unscaled skew in
MergePolicyRecommendations.- Keeps the public
Segment,SegmentMetrics, andMergePolicyRecommendationstypes straightforward and serialization‑friendly.This matches the intended adaptive‑policy integration while preserving a clean, stable
@PublicApisurface.Also applies to: 262-389
|
❌ Gradle check result for a38b165: 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: Sriram Ganesh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.java (2)
26-29: Documentation describes implementation details not present in this class.The javadoc mentions "smooth interpolation (log/linear across size decades)" (lines 27-29), but this logic doesn't exist in
AdaptiveTieredMergePolicyProvider. The class simply initializes a wrappedAdaptiveOpenSearchTieredMergePolicywith static defaults. If the interpolation logic resides in the wrapped policy, update the documentation to clarify that this provider delegates adaptive behavior toAdaptiveOpenSearchTieredMergePolicy, rather than implying this class performs the computation.📝 Suggested documentation clarification
* Implementation notes: - * - Uses smooth interpolation (log/linear across size decades) instead of hard - * categories - * to avoid abrupt parameter jumps as shards grow. + * - Delegates to AdaptiveOpenSearchTieredMergePolicy which uses smooth + * interpolation (log/linear across size decades) to avoid abrupt parameter + * jumps as shards grow. * - Caps the max merged segment size at 5GB to align with Lucene defaults.
59-77: Consider acquiring settingsLock when mutating policy parameters.
applyDefaultSettings()modifiestieredMergePolicystate (lines 65–76) without acquiringsettingsLock. While this is currently safe because the method is only called from the constructor (before the object is published), future refactorings or concurrent access patterns could introduce races. For consistency and defensive coding, consider wrapping the setter calls in a write lock:🔒 Proposed lock usage
private void applyDefaultSettings() { + settingsLock.writeLock().lock(); + try { // Fallback to the original default settings, ensuring parity with // TieredMergePolicyProvider // We use explicit values here, but they match the constants in // TieredMergePolicyProvider: // DEFAULT_MAX_MERGED_SEGMENT = 5GB tieredMergePolicy.setMaxMergedSegmentMB(5 * 1024); // DEFAULT_FLOOR_SEGMENT = 16MB tieredMergePolicy.setFloorSegmentMB(16); // DEFAULT_SEGMENTS_PER_TIER = 10.0 tieredMergePolicy.setSegmentsPerTier(10.0); // DEFAULT_MAX_MERGE_AT_ONCE = 30 tieredMergePolicy.setMaxMergeAtOnce(30); // DEFAULT_EXPUNGE_DELETES_ALLOWED = 10.0 tieredMergePolicy.setForceMergeDeletesPctAllowed(10.0); // DEFAULT_DELETES_PCT_ALLOWED = 20.0 tieredMergePolicy.setDeletesPctAllowed(20.0); tieredMergePolicy.setNoCFSRatio(TieredMergePolicy.DEFAULT_NO_CFS_RATIO); + } finally { + settingsLock.writeLock().unlock(); + } }server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
246-262: Consider tracking segments by identity rather than size value.Line 253 uses
trackedSizes.remove(Long.valueOf(info.sizeInBytes())), which removes by object equality. If multiple segments have identical sizes, this will only remove the first matching value, potentially causing tracking drift. While collisions may be rare due to the random noise added at line 221, using a different data structure (e.g., parallel tracking with indices or a Map) would be more robust.🔎 Alternative approach using indexed tracking
Replace
trackedSizesList with a Map that tracks segment name to size:- List<Long> trackedSegmentSizes = new ArrayList<>(); + Map<String, Long> trackedSegmentSizes = new HashMap<>(); // ... for (int i = 0; i < 5; i++) { long size = 50L * 1024 * 1024; String name = "_init_" + i; segmentInfos.add(createSegmentCommitInfo(name, size)); - trackedSegmentSizes.add(size); + trackedSegmentSizes.put(name, size); } // ... // When adding new segment: - trackedSegmentSizes.add(newSegmentSize); + trackedSegmentSizes.put(newSegmentName, newSegmentSize); // In applyMerges: for (SegmentCommitInfo info : merge.segments) { newSize += info.sizeInBytes(); infos.remove(info); - trackedSizes.remove(Long.valueOf(info.sizeInBytes())); + trackedSizes.remove(info.info.name); } - trackedSizes.add(newSize); + trackedSizes.put(mergedName, newSize); // When calculating metrics: - SegmentTopologyMetrics metrics = calculateTopologyMetrics(trackedSegmentSizes); + SegmentTopologyMetrics metrics = calculateTopologyMetrics(new ArrayList<>(trackedSegmentSizes.values()));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.javaserver/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
🔇 Additional comments (17)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.java (3)
34-39: LGTM!Field declarations are correct. All fields are properly marked
final, and thesettingsLockprovides thread-safety for concurrent merge policy modifications as intended.
41-57: LGTM!Constructor correctly initializes the adaptive policy with thread-safe lock, reads the merge-enabled setting, logs appropriate warnings, and applies defaults.
79-82: LGTM!The
getMergePolicy()method correctly returns the adaptive policy when merges are enabled, orNoMergePolicyotherwise. The conditional logic is clear and the use of fully qualified names improves readability.server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (14)
1-44: LGTM! Clean test class structure.The imports, class declaration, and javadoc are well-organized. The past issue regarding the
@opensearch.apitag has been properly addressed.
50-56: LGTM! Good coverage of shard size scenarios.The test appropriately validates recommendations across a representative range of shard sizes from 50MB to 50GB.
61-69: LGTM! Clear validation of topology quality metrics.The test effectively demonstrates that optimal topologies produce lower variance and better balance scores compared to pathological cases.
126-136: LGTM! Thorough boundary condition testing.The test properly validates all category boundaries, ensuring the categorization logic works correctly at the threshold values.
141-161: LGTM! Comprehensive regression protection.The test validates all merge policy parameters with exact expected values, providing strong regression protection. The previous issue regarding max segment validation has been properly addressed.
163-179: LGTM! Solid sanity checks for recommendations.The validation method appropriately verifies that recommendations are positive, within expected bounds, and maintain proper relationships (e.g., floor < max).
181-198: LGTM! Effective topology simulation for benchmarking.The bad and optimal topology scenarios clearly demonstrate the impact of segment distribution on variance and balance metrics, providing good test coverage.
264-291: LGTM! Proper mock setup for segment simulation.The method correctly creates mock segments with realistic file structure and size reporting, suitable for testing merge policy behavior.
293-303: LGTM! Clear categorization logic.The shard size categorization is straightforward and aligns with the boundaries documented in the javadoc (lines 121-124).
305-336: LGTM! Comprehensive validation with regression protection.The method properly validates all merge policy parameters with both sanity checks and exact value matching. The previous issue regarding missing max segment validation has been resolved.
338-352: LGTM! Numerically sound variance calculation.The method correctly calculates variance using double precision to avoid overflow, addressing the previous review concern.
354-362: LGTM! Intuitive balance score calculation.The balance score formula (1 - range/max) provides an intuitive metric where 1.0 represents perfect balance and lower values indicate increasing imbalance.
364-395: LGTM! Clean inner class definitions.The inner classes are well-structured data holders appropriate for test metrics. The
SegmentTopologyMetricsclass correctly uses double for variance as addressed in previous reviews.
83-116: The adaptive policy is properly configured through segment topology at merge time.The
AdaptiveOpenSearchTieredMergePolicyis designed to dynamically compute shard size from segment metadata in thefindMerges()method rather than requiring explicit configuration at construction. The test properly exercises this behavior:simulateMergePerformance()creates realisticSegmentInfosand invokesfindMerges(), which triggers the adaptive logic to recalculate merge settings based on the current segment topology. No additional shard size configuration is needed or expected.Likely an incorrect or invalid review comment.
|
❌ Gradle check result for e295923: 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: Sriram Ganesh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (2)
221-227: Minor typo in comment.Line 222: "Flashing" should be "Flushing".
🔎 Proposed fix
for (int i = 0; i < iterations; i++) { - // 1. Simulate Flashing a new segment + // 1. Simulate Flushing a new segment long newSegmentSize = docsPerFlush * baseDocSize + randomIntBetween(0, 1024 * 1024); // Add some noise
340-354: Consider adding empty list guard for consistency.
calculateBalanceScorehandles the empty list case (line 357), butcalculateTopologyMetricswould throwArithmeticExceptionon line 342 if passed an empty list. While current callers always pass non-empty lists, adding a guard would be more defensive.🔎 Proposed fix
private SegmentTopologyMetrics calculateTopologyMetrics(List<Long> segmentSizes) { + if (segmentSizes.isEmpty()) { + return new SegmentTopologyMetrics(0.0, 0.0); + } long totalSize = segmentSizes.stream().mapToLong(Long::longValue).sum(); long meanSize = totalSize / segmentSizes.size();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.javaserver/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
🔇 Additional comments (3)
server/src/main/java/org/opensearch/index/AdaptiveTieredMergePolicyProvider.java (1)
1-88: LGTM! Well-structured adaptive merge policy provider.The implementation correctly addresses the concerns from previous reviews:
- Uses
INDEX_MERGE_ENABLEDconstant instead of hardcoded string- Removed dead code (categorization methods, unused settings update method)
- Proper thread-safety with
ReentrantReadWriteLockpassed to the underlying policy- Default values align with
TieredMergePolicyProvider(5GB max segment, 16MB floor, etc.)The lock pattern in
applyDefaultSettings()with try-finally is correct, and returning the policy reference fromgetMergePolicy()without locking is safe since bothmergesEnabledandtieredMergePolicyare final fields set once in the constructor.server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (2)
46-163: Well-structured benchmark tests with good regression coverage.The test file properly addresses the previous review feedback:
- Removed the
@opensearch.apitag from the test class- Variance calculation now uses
doubleto prevent overflow (lines 344-349)- Added
expectedMaxSegmentparameter tovalidateMergeSettingsfor complete regression protection (lines 307-338)The tests provide good coverage for validating the adaptive merge policy across different shard sizes and comparing performance characteristics between default and adaptive policies.
83-85: Remove the unnecessary@SuppressForbiddenannotation.The test method does not call any forbidden APIs. The
@Seedannotation is from the randomizedtesting library and is not subject to forbidden-apis restrictions. The@SuppressForbiddenannotation serves no purpose here and should be removed.@Seed("DEADBEEF") public void testPerformanceComparison() throws IOException {Likely an incorrect or invalid review comment.
|
❌ Gradle check result for e40dce4: TIMEOUT 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: Sriram Ganesh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (1)
server/src/test/java/org/opensearch/index/analysis/SegmentTopologyTestUtils.java (1)
SegmentTopologyTestUtils(19-53)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: detect-breaking-change
🔇 Additional comments (6)
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java (6)
1-46: LGTM!License header, imports, and class declaration are well-structured. The
@opensearch.apitag removal from the previous review has been addressed.
52-71: LGTM!Test coverage for recommendations validation and topology variance impact is well-structured with clear assertions.
131-166: LGTM!The categorization boundary tests and merge settings validation provide solid regression protection. The previous review feedback about adding
expectedMaxSegmenthas been addressed with exact values now being validated for all three parameters.
269-296: LGTM!The
createSegmentCommitInfohelper correctly sets up the mockDirectoryto handle the.cfefile extension, andsetFiles()is consistent with this mock configuration. The hardcoded zero values for delete counts are appropriate for this simulation context.
343-370: LGTM!The variance overflow issue from the previous review has been addressed by using
doublearithmetic. The balance score calculation and edge case handling are correct.
372-403: LGTM!The inner data classes are well-structured for test-only use. The
SegmentTopologyMetricsclass now correctly usesdoublefor variance, addressing the previous review feedback.
| @Seed("DEADBEEF") | ||
| @SuppressForbidden(reason = "Benchmark requires deterministic seed") | ||
| public void testPerformanceComparison() throws IOException { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the complete test method to check for forbidden APIs
cat -n server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java | head -150Repository: opensearch-project/OpenSearch
Length of output: 8329
🏁 Script executed:
#!/bin/bash
# Search for forbidden-apis configuration in OpenSearch
fd -t f "forbidden" -E ".git" -E "target" | head -20Repository: opensearch-project/OpenSearch
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Find examples of @SuppressForbidden usage to understand what triggers it
rg "@SuppressForbidden" --type java -B2 -A5 | head -100Repository: opensearch-project/OpenSearch
Length of output: 12759
🏁 Script executed:
#!/bin/bash
# Check if this class uses any System.* or other commonly forbidden APIs
rg "System\.(getenv|exit|setSecurityManager|setOut|setErr|setIn)" server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.javaRepository: opensearch-project/OpenSearch
Length of output: 55
Remove @SuppressForbidden annotation—no forbidden APIs are used.
The method contains no forbidden API calls (no System.*, Thread.suspend/resume, HttpServer, etc.). The @SuppressForbidden annotation is designed to suppress legitimate forbidden API usage; applying it with a reason about deterministic seeds is incorrect. The @Seed annotation from randomizedtesting is not a forbidden API and requires no suppression.
🤖 Prompt for AI Agents
In server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
around lines 83 to 85, remove the unnecessary @SuppressForbidden annotation on
the testPerformanceComparison method; the method does not use any forbidden APIs
and the @Seed annotation is sufficient for deterministic seeding. Delete the
@SuppressForbidden line and its reason text so only @Seed("DEADBEEF") remains
above the method signature.
server/src/test/java/org/opensearch/index/SegmentTopologyBenchmarkTests.java
Show resolved
Hide resolved
|
❌ Gradle check result for da9f024: null 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: Sriram Ganesh <[email protected]>
2 participants
Description
Introduced an adaptive merge policy system in OpenSearch that dynamically tunes Lucene segment merge settings based on shard size. The solution replaces static defaults with context-aware configurations, improving benchmark consistency and search performance across varying index sizes. Key additions include an
AdaptiveTieredMergePolicyProviderfor shard-specific optimisations, aSegmentTopologyAnalyzerfor segment distribution analysis, and a REST API for real-time monitoring. This change delivers more predictable performance and optimised search behaviour for users.Related Issues
Resolves #11163
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
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.