-
Notifications
You must be signed in to change notification settings - Fork 9.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
[3.4] etcdserver: add more detailed traces on linearized reading #14240
Conversation
125d576
to
f298598
Compare
etcdserver/v3_server.go
Outdated
trace.AddField(traceutil.Field{Key: "readStateIndex", Value: index}) | ||
|
||
ai := s.getAppliedIndex() | ||
trace.AddField(traceutil.Field{Key: "appliedIndex", Value: strconv.FormatUint(ai, 10)}) |
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.
Just a minor comment, why do we need to use strconv.FormatUint
here, but use the original uint64
value (line 792) above?
I understand you just backport the PR, so this comment should be added for the original PR.
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.
I suggest to make them consistent, and update it in the main branch as well.
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.
I agree. slow request logging should also be consistent with metric proposalsApplied.Set(float64(ep.appliedi))
and etcdctl endpoint status output. Not to mention those two index logically are the same. Both points to different positions of raft log.
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 just a backporting PR, so it might be better to merge it for now, and address the comment in separate PR(s).
Are you updating this PR? If not, then let me merge this PR firstly. If you are in progress of updating this PR, then feel free to continue to do it.
To improve debuggability of `agreement among raft nodes before linearized reading`, we added some tracing inside `linearizableReadLoop`. This will allow us to know the timing of `s.r.ReadIndex` vs `s.applyWait.Wait(rs.Index)`. Signed-off-by: Chao Chen <chaochn@amazon.com>
Signed-off-by: Chao Chen <chaochn@amazon.com>
116d5f4
to
864006b
Compare
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.
LGTM
Thank you @chaochn47
Since it's just a minor change, and there is no any change on the functionality, so merging it right now. |
Cherry-pick #12335
ref. #14232
@ahrtr