Ignore Recovery Rate limiter for the warm indexes while download #20109
Conversation
ajaymovva
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
WalkthroughAdds warm-index awareness to remote segment directory creation by introducing two new Changes
Sequence Diagram(s)sequenceDiagram
participant IndexSettings
participant DirectoryFactory as RemoteSegmentStoreDirectoryFactory
participant BlobRepo as BlobStoreRepository
participant RemoteStore as RemoteStore (IO)
Note over IndexSettings,DirectoryFactory: Caller builds Directory
IndexSettings->>DirectoryFactory: newDirectory(indexSettings, shardPath)
DirectoryFactory->>DirectoryFactory: extract isWarmIndex
alt isWarmIndex == true
DirectoryFactory->>BlobRepo: maybeRateLimitRemoteDownloadTransfersForWarm(InputStream)
BlobRepo->>RemoteStore: wrapped stream (warm-rate-limited)
else isWarmIndex == false
DirectoryFactory->>BlobRepo: maybeRateLimitRemoteDownloadTransfers(InputStream)
BlobRepo->>RemoteStore: wrapped stream (standard rate-limited, may include recovery limiter)
end
DirectoryFactory->>DirectoryFactory: return Directory (warm-aware)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
ajaymovva
force-pushed
the
warm-ratelimiter
branch
from
66ba38b to
a4ffc9a
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/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
69-116: SSE and warm flags are correctly threaded through newDirectory overloadsThe added overloads and the updated
newDirectory(IndexSettings, ShardPath)correctly propagate bothisServerSideEncryptionEnabledandisWarmIndexdown to the core factory method. This keeps existing call sites working while enabling more precise configuration.Given the growing parameter list on the deepest overload, consider introducing a small parameter holder (e.g.,
RemoteDirectoryConfigwith builder-style methods) in a future refactor to reduce the risk of boolean mis-ordering, but this is not a blocker for this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(3 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
server/src/main/java/org/opensearch/index/remote/RemoteStoreUtils.java (1)
RemoteStoreUtils(57-588)
⏰ 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, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: detect-breaking-change
🔇 Additional comments (2)
server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java (1)
4429-4436: Warm-path helper cleanly bypasses recovery rate limiterThe new
maybeRateLimitRemoteDownloadTransfersForWarmcorrectly mirrors the inner part ofmaybeRateLimitRemoteDownloadTransfersand omits the recovery-level rate limiter, which matches the PR goal while keeping metrics and logging consistent.server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
137-148: Warm index data downloads now bypass recovery rate limiter—confirm scope is intentionalThe
RemoteDirectoryfor the data path now selects:
- warm index:
BlobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm(no recovery rate limiter), and- non-warm index: existing
maybeRateLimitRemoteDownloadTransfers(still nested withrecoverySettings.recoveryRateLimiter),while the low-priority download path continues to use
maybeRateLimitLowPriorityDownloadTransfersunchanged.This matches the PR objective for warm data downloads; please double-check that you do not also need to bypass the recovery limiter for any low-priority warm-download flows, or introduce a warm-specific low-priority variant, depending on your traffic patterns.
|
❌ Gradle check result for a4ffc9a: 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: Ajaykumar Movva <[email protected]>
ajaymovva
force-pushed
the
warm-ratelimiter
branch
from
a4ffc9a to
553bfd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
108-116: New overload signature is fine; consider documentingisWarmIndexbehaviorIntroducing the 7‑arg overload on a
@PublicApiclass is a safe, backward‑compatible way to surface warm‑index behavior, and the parameter order is consistent with the previous method.To make usage by plugins/embedders clearer, consider adding a short Javadoc to this overload explaining what
isWarmIndexchanges (i.e., which rate limiters or paths are affected) so downstream users don’t misuse the flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java(3 hunks)server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (1)
server/src/main/java/org/opensearch/index/remote/RemoteStoreUtils.java (1)
RemoteStoreUtils(57-588)
⏰ 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 (25, windows-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 (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java (3)
69-79: Correct wiring of warm index and SSE flags; verify metadata availabilityThe additional parameters propagated from
IndexSettings(isServerSideEncryptionEnabledIndex(indexSettings.getIndexMetadata())andindexSettings.isWarmIndex()) look correct and align with the intent of making this factory warm‑aware.Only concern: this assumes
indexSettings.getIndexMetadata()is always non‑null on this path. If there are any internal/test callers that constructIndexSettingswithout metadata, this would now throw an NPE where previously it did not.If you’re not fully sure about that invariant, consider either:
- guarding with a null‑check and defaulting SSE to
false, or- documenting the requirement that
IndexSettingsmust carryIndexMetadatawhen using remote store directories.
97-106: Backward‑compatible delegation from 6‑arg to 7‑arg overload looks goodThe existing 6‑parameter overload now simply delegates to the new 7‑parameter version with
isWarmIndex=false, which preserves previous behavior for all existing callers.
137-148: Warm‑specific download rate limiter selection matches PR intent; add coverageConditionally selecting
maybeRateLimitRemoteDownloadTransfersForWarmfor warm indices and falling back tomaybeRateLimitRemoteDownloadTransfersotherwise is a minimal, targeted change and keeps upload & low‑priority download behavior intact. This aligns with the stated goal of treating warm‑index downloads differently without impacting other tiers.Given this is performance/throughput‑sensitive behavior, it would be good to have at least one test (unit or integration) that:
- constructs a warm and a non‑warm index, and
- asserts that the appropriate download rate‑limiting callback is wired into
RemoteDirectoryfor each case.
gbbafna
left a comment
gbbafna
left a comment
There was a problem hiding this comment.
Changes looks good to me .
gbbafna
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20109 +/- ##
============================================
- Coverage 73.33% 73.26% -0.07%
+ Complexity 71679 71651 -28
============================================
Files 5790 5786 -4
Lines 327549 327631 +82
Branches 47181 47207 +26
============================================
- Hits 240217 240055 -162
- Misses 68080 68306 +226
- Partials 19252 19270 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nsearch-project#20109) Signed-off-by: Ajay kumar Movva <[email protected]>
…nsearch-project#20109) Signed-off-by: Ajay kumar Movva <[email protected]>
2 participants
Description
Ignoring Recovery Ratelimiter for the warm indexes while download
Related 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
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.