improve replication/handover check logging and readability#9844
improve replication/handover check logging and readability#9844thestephenstanton merged 10 commits intomainfrom
Conversation
|
|
| shardID: localShard.GetShardId(), | ||
| laggingTasks: laggingTasks, | ||
| timeLag: timeLag, | ||
| isReady: fullyCaughtUp || (passedRequiredMinimum && withinLagTolerance), |
There was a problem hiding this comment.
For a future PR: fullyCaughtUp guarantees withinLagTolerance is true, but I don't want the scope of this PR to get too huge.
| isReady := notReadyShardCount == 0 | ||
|
|
||
| if !isReady { | ||
| a.logger.Info("Wait catchup not ready", |
There was a problem hiding this comment.
(Future PR problem) This feels like it wants a "TotalTimeSpentWaiting" kind of tag, since we expect to be here about 1/sec
…r first main loop
thestephenstanton
force-pushed
the
sstanton/cleaner-more-detailed-catchup-funcs
branch
from
25a051e to
536018c
Compare
| // Check if remote cluster has caught up on all shards on replication tasks from target replica. | ||
| func (a *activities) checkReplicationOnRemoteCluster(ctx context.Context, waitRequest waitCatchupRequest, targetAckIDOnShard map[int32]int64) (bool, error) { | ||
|
|
||
| func (a *activities) checkReplicationOnRemoteCluster(ctx context.Context, waitRequest waitCatchupRequest, requiredMinTaskIDPerShard map[int32]int64) (bool, error) { |
There was a problem hiding this comment.
Future PR problem: Figure out what's different between this and checkHandoverOnce, and deduplicate these functions
| tag.Int64("ActualLaggingTasks", shard.MaxReplicationTaskId-clusterInfo.AckedTaskId), | ||
| ) | ||
| // If the target acked task ID is NOT found, the shard is considered ready, as the remote ack level | ||
| // is assumed to be more up-to-date than the active ack level. |
There was a problem hiding this comment.
Future PR problem: This comment is pretty vague. Why is this ok? How does "more up to date" imply that the task ID might be missing? I have some guesses as to why, but we should write it authoritatively here.
thestephenstanton
force-pushed
the
sstanton/cleaner-more-detailed-catchup-funcs
branch
from
536018c to
40b84bb
Compare
thestephenstanton
deleted the
sstanton/cleaner-more-detailed-catchup-funcs
branch
4 participants
Continuing from this PR: #9787
What changed?
Replaced the log dump that happened on the first non ready shard with with a single summary log per invocation that includes total, ready, and not ready shard counts and the slowest shard by task lag and time lag, making it easier to diagnose stalled replication and handover.
Refactored
checkReplicationOnce,checkHandoverOnce, andcheckReplicationOnRemoteClusterto use guard clauses, eliminating nested logic and the logged guard pattern while also renaming some variables; all for improved readability.Also, there is a fix to a subtle bug where if the remote cluster shard progress lookup (map inside the shard loop) doesn't have data and we have already logged previous non ready shards, then we would actually not return an error.
Why?
Log change: The old log only captured one shard's state at a time, making it hard to understand overall progress during a stalled catchup or handover. A single summary with counts and the slowest shards gives you the full picture in one entry.
Refactor: The nested if/logged guard pattern made it harder to follow than needed; guarding flattens this out vs having deep nests. Also renamed variables make things more clear.
How did you test it?
Potential risks
If anyone is extremely dependent on the previous log fields.