kvcoord: include replica info in RangeIterator.Seek into trace#92947
kvcoord: include replica info in RangeIterator.Seek into trace#92947craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks)
|
TFTR! bors r+ |
AlexTalks
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/range_iter.go line 183 at r1 (raw file):
func (ri *RangeIterator) Seek(ctx context.Context, key roachpb.RKey, scanDir ScanDirection) { if log.HasSpanOrEvent(ctx) { defer func() {
Question though: why defer this? once we get to ri.ds.getRoutingInfo(..) we are looking up (in a retry loop) the range info in the range cache, which may involve a KV lookup of meta1/meta2 - i.e. a potentially slow RPC. I I assume it's because you want to incorporate the range info, but wouldn't it be better to potentially do log.Eventf(..) both here and after the lookup?
|
Let me think about that. bors r- |
|
Canceled. |
c7c1fd0 to
851c7a0
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/range_iter.go line 183 at r1 (raw file):
Previously, AlexTalks (Alex Sarkesian) wrote…
Question though: why defer this? once we get to
ri.ds.getRoutingInfo(..)we are looking up (in a retry loop) the range info in the range cache, which may involve a KV lookup ofmeta1/meta2- i.e. a potentially slow RPC. I I assume it's because you want to incorporate the range info, but wouldn't it be better to potentially dolog.Eventf(..)both here and after the lookup?
I didn't want to pollute the trace too much, but you raise a good point that we might be interested to know how long this Seek actually takes. Thus, I changed the code to replace the existing "info" log message into a trace event. That "info" message was introduced in #15413, and probably hasn't been very useful, but please let me know if you think otherwise.
AlexTalks
left a comment
There was a problem hiding this comment.
Reviewable status:
This commit modifies the existing "info" log message into an "event"
when seeking the range iterator. This makes it so that the result of the
seek (the replica information) is included into the trace. Additionally,
this commit includes the corresponding message to be included into the
KV trace. The original "info" log message was added about five years ago
and probably hasn't been that useful.
Here is an example of the trace event:
```
key: /NamespaceTable/30/1/100/101/"t"/4/1, desc: r32:/NamespaceTable/{30-Max} [(n1,s1):1, next=2, gen=0]
```
Release note: None
851c7a0 to
c1ae40b
Compare
|
Needed to adjust one logic test file. TFTRs! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from c1ae40b to blathers/backport-release-22.1-92947: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
This commit modifies the existing "info" log message into an "event"
when seeking the range iterator. This makes it so that the result of the
seek (the replica information) is included into the trace. Additionally,
this commit includes the corresponding message to be included into the
KV trace. The original "info" log message was added about five years ago
and probably hasn't been that useful.
Here is an example of the trace event:
Informs: https://github.com/cockroachlabs/support/issues/1933.
Release note: None