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

pdpb: Add err type and input param to BatchScanRegions #1252

Merged
merged 7 commits into from
Jul 16, 2024

Conversation

okJiang
Copy link
Member

@okJiang okJiang commented Jul 9, 2024

Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Copy link
Member Author

Choose a reason for hiding this comment

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

Automatically generated

proto/pdpb.proto Outdated
@@ -377,6 +378,9 @@ message BatchScanRegionsRequest {

repeated KeyRange ranges = 3; // the given ranges must be in order.
int32 limit = 4; // limit the total number of regions to scan.
// If output_must_contain_all_key_range is true, the output must contain all key ranges in the request.
// If the output does not contain all key ranges, the request is considered failed and returns an error(REGIONS_NOT_CONTAIN_ALL_KEY_RANGE).
bool output_must_contain_all_key_range = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it shorter?

Copy link
Member Author

@okJiang okJiang Jul 9, 2024

Choose a reason for hiding this comment

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

Do you have any better suggestions?😂

Copy link
Member

Choose a reason for hiding this comment

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

contain_all_key_range?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think which is better? @lance6716 @you06

Copy link
Contributor

Choose a reason for hiding this comment

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

With the comment explaination, contain_all_key_range is clear enough to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@you06 updated in 49bf52b

proto/pdpb.proto Outdated Show resolved Hide resolved
Co-authored-by: Neil Shen <[email protected]>
@okJiang
Copy link
Member Author

okJiang commented Jul 15, 2024

If you have no question, could you please give me an approve? @you06

Copy link

ti-chi-bot bot commented Jul 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rleungx, you06

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

ti-chi-bot bot commented Jul 16, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-07-12 06:56:22.573620911 +0000 UTC m=+599879.808855023: ☑️ agreed by rleungx.
  • 2024-07-16 09:52:13.41516338 +0000 UTC m=+347555.406104850: ☑️ agreed by you06.

@ti-chi-bot ti-chi-bot bot merged commit 5f7ffec into pingcap:master Jul 16, 2024
5 checks passed
@okJiang okJiang deleted the add-err-type branch July 16, 2024 10:08
@ekexium
Copy link
Contributor

ekexium commented Sep 6, 2024

We'd better merge such changes together with the related TiKV PRs to prevent breaking TiKV's build when using the master kvproto.

@okJiang
Copy link
Member Author

okJiang commented Sep 6, 2024

We'd better merge such changes together with the related TiKV PRs to prevent breaking TiKV's build when using the master kvproto.

got it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants