Introduce a unified download manager for remote store operations #12071
Conversation
github-actions
bot
added
enhancement
Compatibility status:Checks if related components are compatible with change a8af76a Incompatible componentsIncompatible components: [https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/performance-analyzer.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git] |
|
❌ Gradle check result for bc7705b: 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? |
| this.recoverySettings = recoverySettings; | ||
| } | ||
|
|
||
| public DownloadManager(DownloadManager other) { |
There was a problem hiding this comment.
Do you really need a copy constructor?
There was a problem hiding this comment.
The wrapper class StatsTrackingDownloadManager is created within the Translog construct. Copy constructor seems to be a cleaner approach than exposing getter/setters.
server/src/main/java/org/opensearch/common/transfermanager/download/DownloadManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/transfermanager/download/DownloadManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/transfermanager/download/DownloadManager.java
Outdated
Show resolved
Hide resolved
kotwanikunal
force-pushed
the
download-manager
branch
from
bc7705b to
5b9ad7d
Compare
|
❌ Gradle check result for 5b9ad7d: 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? |
kotwanikunal
force-pushed
the
download-manager
branch
from
5b9ad7d to
eceb712
Compare
kotwanikunal
requested review from
abbashus,
adnapibar,
anasalkouz,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
dreamer-89,
gbbafna,
kartg,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
ryanbogan,
sachinpkale,
saratvemulapalli and
shwetathareja
as code owners
kotwanikunal
requested review from
sohami,
tlfeng and
VachaShah
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12071 +/- ##
============================================
- Coverage 71.26% 71.23% -0.04%
- Complexity 59413 59448 +35
============================================
Files 4927 4926 -1
Lines 279662 279538 -124
Branches 40656 40645 -11
============================================
- Hits 199311 199125 -186
- Misses 63725 63848 +123
+ Partials 16626 16565 -61 ☔ View full report in Codecov by Sentry. |
|
@Bukhtawar / @gbbafna Can you please have a look? |
| @PublicApi(since = "2.11.0") | ||
| public final class RemoteStoreFileDownloader { | ||
| private final Logger logger; | ||
| @PublicApi(since = "2.13.0") |
There was a problem hiding this comment.
This is labelled as public api, the renames are breaking changes. Think we need make a new class and deprecate this one?
There was a problem hiding this comment.
Can we audit that the PublicApi annotation is correct here? It surprises me that this would be exposed through any plugin-accessible interfaces.
There was a problem hiding this comment.
I think this was a miss with the previous code push. We might have to keep RemoteStoreFileDownloader until 3.0.0 in that case?
server/src/main/java/org/opensearch/index/remote/transfer/DownloadManager.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kunal Kotwani <[email protected]>
kotwanikunal
force-pushed
the
download-manager
branch
from
eceb712 to
a8af76a
Compare
|
❌ Gradle check result for a8af76a: 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? |
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
Description
Related Issues
Resolves #11928
Check List
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)Commit changes are listed out in CHANGELOG.md file (See: Changelog)Public documentation issue/PR createdBy 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.