Implemented computation of segment replication stats at shard level #17055
Conversation
The method implemented here computes the segment replication stats at the shard level, instead of relying on the primary shard to compute stats based on reports from its replicas. Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
vinaykpud
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
jainankitk and
linuxpi
as code owners
github-actions
bot
added
enhancement
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
|
❌ Gradle check result for dd0406d: 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 dd0406d: 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: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
|
❌ Gradle check result for 59e2617: 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 59e2617: 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 59e2617: 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 59e2617: 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 59e2617: 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? |
…ibility Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17055 +/- ##
============================================
+ Coverage 72.19% 72.37% +0.17%
- Complexity 65304 65385 +81
============================================
Files 5301 5301
Lines 303774 303817 +43
Branches 44034 44040 +6
============================================
+ Hits 219323 219891 +568
+ Misses 66458 65882 -576
- Partials 17993 18044 +51 ☔ View full report in Codecov by Sentry. |
...c/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationStatsIT.java
Show resolved
Hide resolved
| @@ -149,7 +150,8 @@ private static ReplicationCheckpoint readCheckpointFromIndexInput( | |||
| in.readLong(), | |||
| in.readLong(), | |||
| in.readString(), | |||
| toStoreFileMetadata(uploadedSegmentMetadataMap) | |||
| toStoreFileMetadata(uploadedSegmentMetadataMap), | |||
| in.readLong() | |||
There was a problem hiding this comment.
How does bwc work here with this added field? Do we need to bump CURRENT_VERSION on RemoteSegmentMetadata ? @sachinpkale can you pls assist?
There was a problem hiding this comment.
If data is being read without the writer serialising it, we will end with an end of stream exception
There was a problem hiding this comment.
right, so we'd need to check the version before attempting to read. I think we may want to add a mixed cluster test here in any case. I don't think theres an existing qa pkg that runs with remote enabled clusters.
| @@ -294,6 +294,7 @@ public synchronized void onNewCheckpoint(final ReplicationCheckpoint receivedChe | |||
| return; | |||
| } | |||
| updateLatestReceivedCheckpoint(receivedCheckpoint, replicaShard); | |||
There was a problem hiding this comment.
We should be able to remove this and the latestReceivedCheckpoint map from this class as the replicator is the source of truth now.
| ; | ||
| final Map<String, StoreFileMetadata> indexStoreFileMetadata = indexReplicationCheckPoint.getMetadataMap(); | ||
| // If primaryLastRefreshedCheckpoint is null, we will default to indexReplicationCheckPoint | ||
| // so that we can avoid any failures |
There was a problem hiding this comment.
when does this happen? In this case can we not assume the shard is up to date and return empty ReplicationStats obj instead?
| ); | ||
| final Map<String, StoreFileMetadata> storeFileMetadata = primaryLastRefreshedCheckpoint.getMetadataMap(); | ||
|
|
||
| final Store.RecoveryDiff diff = Store.segmentReplicationDiff(storeFileMetadata, indexStoreFileMetadata); |
There was a problem hiding this comment.
Rather than storing two ReplicationCheckpoint maps, lets store a single ShardId to ReplicationStats map and compute the stats as information about new primary checkpoints is received.
I'm also not following the lastOnGoingReplicationCheckpoint computation because cp used to compute bytes behind is not the same as the one used for lag. To simplify I think we only care about the primarylastRefreshed and should compute both stats against that, where lag is computed as the time between primarylastRefreshed and the currently searchable checkpoint on the shard.
This would still fall in line with our definition of 'Replication lag' in docs
| segments.segment_replication.max_replication_lag | long | The maximum amount of time, in milliseconds, taken by a replica to catch up to its primary. |
|---|
There was a problem hiding this comment.
As we discussed,
- Lets compute the diff(bytesBehind) on the go everytime when we receive the primary refreshed checkpoint
- Along with that, instead of maintaining two different maps, we can try to combine them in a single map
ex: Map<ShardId, Map<SegmentInfoVersion, ComputedDiff>
class ComputedDiff {
long timeStamp,
long bytesBehind
}
-
Once the checkpoint is computed we can remove all the entries less than replication completed checkpoint segmentInfoVersion
-
when nodeStats requested we can use the diff from the map and calculate the lag based on currentTime and timeStamp in the ComputedDiff
There was a problem hiding this comment.
This works, we are doing similar tracking now with ReplicationTracker on the primary. You will need to wire this up with Index events IndexEventListener through the segrep service so that shard entries are cleared on removal.
Description
The method implemented here computes the segment replication stats at the shard level, instead of relying on the primary shard to compute stats based on reports from its replicas.
Method implemented in this PR serves the segment replication stats for following core APIs:
/_nodes/stats)/_cluster/stats)/_statsor/{index}/_stats)Related Issues
Resolves #16801
Related to #15306
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.