Skip to content
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

kv: refactor CompareWithLocality to use enum #105267

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Jun 21, 2023

Prior to this commit, CompareWithLocality returned two boolean values
indicating whether two localities were cross-region and cross-zone. However,
this required callers to perform additional cross-comparison of these boolean
values to make meaningful metrics updates.

To simplify this, this commit introduces a new enum type
LocalityComparisonType. It provides four locality comparison results: cross
region, same region cross zone, same region same zone, and undefined
(indicating error behavior). This refactoring allow the caller to directly use
the comparison result without additional operations.

In addition, this commit also updates the logic to classify activities between
different regions as cross-regional, regardless of the zone tiers’
configuration. Initially, cross-region but same-zone tiers activities were
flagged as misconfiguration. After some discussion, we have decided that
regions should be non-overlapping. Hence, same zone tiers from different
regions should still be considered as different zones.

In addition, this commit also includes some refactoring of function
parameters.

Note that this commit does not change any existing functionality.

Part of: #103983

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6 wenyihu6 changed the title Remove tests kv: refactor metric updates test cases Jun 21, 2023
@wenyihu6 wenyihu6 force-pushed the remove-tests branch 3 times, most recently from 75363c5 to cf982e9 Compare June 21, 2023 12:05
@wenyihu6 wenyihu6 marked this pull request as ready for review June 21, 2023 12:05
@wenyihu6 wenyihu6 requested review from a team as code owners June 21, 2023 12:05
@wenyihu6 wenyihu6 requested a review from tbg June 21, 2023 12:05
@wenyihu6 wenyihu6 force-pushed the remove-tests branch 4 times, most recently from 1eb7aaf to d4322bc Compare June 22, 2023 00:14
@wenyihu6 wenyihu6 self-assigned this Jun 22, 2023
@wenyihu6 wenyihu6 force-pushed the remove-tests branch 2 times, most recently from 8a7f737 to da864f0 Compare June 22, 2023 07:18
Copy link
Collaborator

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: last commit

Reviewed 3 of 8 files at r1, 3 of 6 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg and @wenyihu6)


pkg/kv/kvclient/kvcoord/dist_sender.go line 2588 at r3 (raw file):

	gatewayNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(fromNodeID)
	if err != nil {
		log.VEventf(ctx, 2, "failed to perform look up for node descriptor %+v", err)

Why %+v and not %v?

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Only small changes.

This reviews both commits. There is a lot of churn between the two so that in my opinion, they don't make sense as standalone commits. A better sequence of commits is possible, but there are few benefits to introducing it now.

So my suggestion would be to squash the current two commits with an updated message (and PR description) and closing the old PR (that contains the first commit only).
Please add fix-up commits as new commits (only to be squashed immediately before merge, to save me another full review).

pkg/kv/kvclient/kvcoord/dist_sender_test.go Show resolved Hide resolved
pkg/kv/kvserver/store_snapshot.go Show resolved Hide resolved
pkg/kv/kvclient/kvcoord/dist_sender.go Outdated Show resolved Hide resolved
pkg/roachpb/metadata.proto Outdated Show resolved Hide resolved
@wenyihu6 wenyihu6 force-pushed the remove-tests branch 2 times, most recently from 31bfcc4 to c4eacc9 Compare June 26, 2023 12:40
@wenyihu6
Copy link
Contributor Author

Looking good! Only small changes.

This reviews both commits. There is a lot of churn between the two so that in my opinion, they don't make sense as standalone commits. A better sequence of commits is possible, but there are few benefits to introducing it now.

So my suggestion would be to squash the current two commits with an updated message (and PR description) and closing the old PR (that contains the first commit only). Please add fix-up commits as new commits (only to be squashed immediately before merge, to save me another full review).

Ack. Added a new commit "to be squashed commit" which fixes the comments.

@wenyihu6 wenyihu6 requested a review from tbg June 26, 2023 13:55
@wenyihu6 wenyihu6 force-pushed the remove-tests branch 4 times, most recently from eff5a72 to 7082ec7 Compare June 26, 2023 22:58
@wenyihu6 wenyihu6 changed the title kv: refactor metric updates test cases kv: refactor CompareWithLocality to use enum Jun 28, 2023
@tbg
Copy link
Member

tbg commented Jun 30, 2023

Looking good! Now all that's left is to squash all commits into the first one 💪🏽

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once squashed into the first commit.

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash, on the assumption that the first commit is identical with what I reviewed in the other PR.

Prior to this commit, `CompareWithLocality` returned two boolean values
indicating whether two localities were cross-region and cross-zone. However,
this required callers to perform additional cross-comparison of these boolean
values to make meaningful metrics updates.

To simplify this, this commit introduces a new enum type
`LocalityComparisonType`. It provides four locality comparison results: cross
region, same region cross zone, same region same zone, and undefined
(indicating error behavior). This refactoring allow the caller to directly use
the comparison result without additional operations.

In addition, this commit also updates the logic to classify activities between
different regions as cross-regional, regardless of the zone tiers’
configuration. Initially, cross-region but same-zone tiers activities were
flagged as misconfiguration. After some discussion, we have decided that
regions should be non-overlapping. Hence, same zone tiers from different
regions should still be considered as different zones.

In addition, this commit also includes some refactoring of function
parameters.

Note that this commit does not change any existing functionality.

Part of: cockroachdb#103983

Release note: None
@wenyihu6
Copy link
Contributor Author

Thank you so much for the review, Tobi and Austen! (the last push is for a comment from a separate PR and has been approved.)

bors r=tbg,kvoli

@craig
Copy link
Contributor

craig bot commented Jun 30, 2023

Build succeeded:

@craig craig bot merged commit 66c9f93 into cockroachdb:master Jun 30, 2023
@wenyihu6 wenyihu6 deleted the remove-tests branch October 30, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants