-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Remote Store] Adding Segment download stats to remotestore stats API #8440
[Remote Store] Adding Segment download stats to remotestore stats API #8440
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
...a/org/opensearch/action/admin/cluster/remotestore/stats/TransportRemoteStoreStatsAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/settings/ClusterSettings.java
Outdated
Show resolved
Hide resolved
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.
Is it possible to move the segments download to a different java class. That would not bloat the IndexShard class more.
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentPressureService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentPressureService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentPressureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentTracker.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
if (!indexSettings.isRemoteStoreEnabled()) { | ||
return; |
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.
Why is this check required? The concerned code only runs for remote store enabled indexes.
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.
As discussed, the same copySegmentFiles
method is being used during snapshot restores from remote store also. This is to handle any indices during snapshot restore which has an overriden index setting to disable RemoteStore. Will add a comment here regarding this.
if (!indexSettings.isRemoteStoreEnabled()) { | ||
return; | ||
} | ||
downloadStatsTracker.incrementTotalDownloadsStarted(); |
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.
This is used to track overall downloads and not file level. Lets fix this.
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.
downloadStatsTracker.incrementTotalDownloadsStarted();
should be called only once per invocation of syncSegmentsFromRemoteSegmentStore
.
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.
Ack
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
long currentTimeInNs = System.nanoTime(); | ||
downloadStatsTracker.updateLastDownloadTimestampMs(System.currentTimeMillis()); |
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.
Do we need this file level or per sync level?
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.
Is it possible to move the segments download to a different java class. That would not bloat the IndexShard class more.
Also, please fix the DCO check - https://github.com/opensearch-project/OpenSearch/pull/8440/checks?check_run_id=14908813252 |
builder.endObject(); | ||
builder.endObject(); | ||
} else { | ||
builder.startObject(SubFields.DOWNLOAD); |
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.
So for primary shards we will show download field as {}
.
Can we validate and put some of the existing API which does the same. We just need to make sure that user is well aware that this stats will be empty in case of primary shard and this should not lead to any issues in future.
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.
This is inline with the other APIs also, wherein null or 0 values are being shown as a simple {}
. The shard routing section of the _cluster/state
is an instance
...er/src/main/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStats.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStats.java
Outdated
Show resolved
Hide resolved
.field(SubFields.FAILED, remoteSegmentShardStats.downloadBytesFailed); | ||
builder.endObject(); | ||
builder.startObject(DownloadStatsFields.DOWNLOAD_SIZE_IN_BYTES) | ||
.field(SubFields.LAST_SUCCESSFUL, remoteSegmentShardStats.lastSuccessfulSegmentDownloadBytes) |
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.
Is there a specific reason to have last successfully downloaded segment file size? How does this benefit user from stats perspective?
remoteSegmentShardStats.localRefreshNumber - remoteSegmentShardStats.remoteRefreshNumber | ||
) | ||
.field(UploadStatsFields.BYTES_LAG, remoteSegmentShardStats.bytesLag) | ||
.field(UploadStatsFields.BACKPRESSURE_REJECTION_COUNT, remoteSegmentShardStats.rejectionCount) |
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.
Question : Will there be a scenario where we will have back pressure during download?
Is this something that we need or are we good, thinking from future perspective where n numbers of segments are getting restored or recovered which might lead to some delay or congestion.
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.
As of now we are not putting any backpressure on downloads. IMO the indices.recovery.max_bytes_per_sec
setting is already there to enforce a check on downloads.
...ain/java/org/opensearch/action/admin/cluster/remotestore/stats/RemoteStoreStatsResponse.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentTracker.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/remote/RemoteRefreshSegmentTracker.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
6d49924
to
d826579
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
14d8b2e
to
f282d54
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
9daeb74
to
2f4545a
Compare
Gradle Check (Jenkins) Run Completed with:
|
Description
_remotestore/stats
APISample API outputs from running in local dev setup
Related Issues
Resolves #8395
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.