kv,server,roachpb: avoid error overhead for x-locality comparison#113229
kv,server,roachpb: avoid error overhead for x-locality comparison#113229craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
Cross locality traffic instrumentation was added to raft, snapshots and batch requests to quantify the amount of cross region/zone traffic. Errors would be returned from `CompareWithLocality` when the region, or zone locality flags were set in an unsupported manner according to our documentation. These error allocations added overhead (cpu/mem) when hit. Alter `CompareWithLocality` to return booleans in place of an error to reduce overhead. Resolves: cockroachdb#111148 Resolves: cockroachdb#111142 Informs: cockroachdb#111561 Release note: None
b86e5c0 to
a760335
Compare
|
Benchmarking the comparison of this commit cherry-picked on |
|
|
||
| if !hasRegion || !hasRegionOther { | ||
| isCrossRegion = false | ||
| regionErr = errors.Errorf("localities must have a valid region tier key for cross-region comparison") |
There was a problem hiding this comment.
Alternatively, have you considered making these errors.New instead of errors.Errorf? I wonder if the allocation behaviour is any different in this case. Probably not much better.
There was a problem hiding this comment.
I didn't consider that, I think it might help moderately, but they both seem to end up at errutil.NewWithDepthf(), with depth=1.
| @@ -698,7 +698,7 @@ func (l Locality) getFirstRegionFirstZone() ( | |||
| // iteration. | |||
| func (l Locality) CompareWithLocality( | |||
There was a problem hiding this comment.
Side note (irrelevant to this PR): this function seems unnecessarily complex for what it does. It tries to return something meaningful even in error cases, which is a recipe for combinatorial boost and bugs. The boolean logic is hard to follow.
There was a problem hiding this comment.
Ack, I agree. I considered moving the validation/error logic into a separate function, which would be called on node start / periodically / v(5) return path.
We can clean this up in the future.
|
TYFTR bors r=pavelkalinnikov |
|
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 setting reviewers, but backport branch blathers/backport-release-23.2-113229 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/113302/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
blathers backport 23.2 |
|
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 backport branch refs/heads/blathers/backport-release-23.2-113229: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists [] Backport to branch 23.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Cross locality traffic instrumentation was added to raft, snapshots and batch requests to quantify the amount of cross region/zone traffic. Errors would be returned from
CompareWithLocalitywhen the region, or zone locality flags were set in an unsupported manner according to our documentation. These error allocations added overhead (cpu/mem) when hit.Alter
CompareWithLocalityto return booleans in place of an error to reduce overhead.Resolves: #111148
Resolves: #111142
Informs: #111561
Release note: None