api: supports `/regions/key` by hex key #8262

HuSharp · 2024-06-06T05:06:57Z

What problem does this PR solve?

Issue Number: Close #8261

What is changed and how does it work?

api: supports `/regions/key` by hex key

Check List

Tests

Unit test

Release note

None.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot · 2024-06-06T05:07:00Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

HuSharp · 2024-06-06T05:07:52Z

PTAL, thx! @yongman

codecov · 2024-06-06T05:16:07Z

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.33%. Comparing base (e52f5be) to head (364b209).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8262      +/-   ##
==========================================
- Coverage   77.41%   77.33%   -0.09%     
==========================================
  Files         471      471              
  Lines       61395    61405      +10     
==========================================
- Hits        47529    47486      -43     
- Misses      10314    10363      +49     
- Partials     3552     3556       +4

Flag	Coverage Δ
unittests	`77.33% <87.50%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

JmPotato

Can we add this new change to the PD HTTP SDK as well?

server/api/region.go

ti-chi-bot · 2024-06-06T05:20:37Z

@yongman: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

HuSharp · 2024-06-06T05:24:49Z

Can we add this new change to the PD HTTP SDK as well?

The GetRegionsByKeyRange interface of the PD HTTP SDK only supports bytes, as does GetRegionByKey. Do we need hex support for both interfaces?

Signed-off-by: husharp <jinhao.hu@pingcap.com>

okJiang

rest lgtm

okJiang · 2024-06-06T07:10:23Z

pkg/utils/apiutil/apiutil.go

@@ -336,6 +336,22 @@ func ParseKey(name string, input map[string]any) ([]byte, string, error) {
 	return returned, rawKey, nil
 }

+// ParseHexKeys decodes hexadecimal keys to bytes if the format is "hex".
+func ParseHexKeys(format string, keys ...*string) error {


Returning parsed key to user is better than returning result by input parameter implicitly.

Suggested change

func ParseHexKeys(format string, keys ...*string) error {

func ParseHexKeys(format string, keys []string) ([]string, error) {

want to reduce redundant code :(

I know what you want... But this suggestion aims to avoid to changing data in the input implicitly, and each update operation, I think, should re-assign to the old value. Let's see what other reviewers think.

Perhaps there is no need to provide a function. Because of the multiple arguments and the error check which will make code more redundant.

cc @JmPotato

This involves converting between []byte and string. I prefer a more performant approach; a benchmark may help.

pkg/utils/apiutil/apiutil.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

okJiang · 2024-06-07T06:57:35Z

pkg/utils/apiutil/apiutil.go

@@ -336,6 +336,22 @@ func ParseKey(name string, input map[string]any) ([]byte, string, error) {
 	return returned, rawKey, nil
 }

+// ParseHexKeys decodes hexadecimal keys to bytes if the format is "hex".
+func ParseHexKeys(format string, keys []string) (hexStrings []string, err error) {


Suggested change

func ParseHexKeys(format string, keys []string) (hexStrings []string, err error) {

func ParseHexKeys(format string, keys []string) (hexBytes [][]byte, err error) {

returning bytes can reduce some converting

changed in 3f515a6
which change hex.String to hex.Decode, PTAL, thx!

ti-chi-bot · 2024-06-07T06:57:57Z

@okJiang: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

rleungx · 2024-06-13T04:14:40Z

pkg/utils/apiutil/apiutil.go

+	if format != "hex" {
+		return keys, nil
+	}


Shall we move it out of this function?

No, we only use it when the format is "hex" and want to extract common logic.
add more comment to ParseHexKeys, PTAL, thx!
ref #8262 (comment)

Since the function is named ParseHexKeys, I think it has already be in hex format before calling this function.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot · 2024-06-13T06:35:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, nolouch, okJiang, yongman

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:

~~OWNERS~~ [JmPotato,nolouch]

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

ti-chi-bot · 2024-06-13T06:35:53Z

[LGTM Timeline notifier]

Timeline:

2024-06-13 03:51:50.345656805 +0000 UTC m=+609464.398968722: ☑️ agreed by nolouch.
2024-06-13 06:35:52.232052406 +0000 UTC m=+619306.285364329: ☑️ agreed by JmPotato.

add hex

b173483

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp requested review from JmPotato and lhy1024 June 6, 2024 05:06

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024

JmPotato reviewed Jun 6, 2024

View reviewed changes

server/api/region.go Outdated Show resolved Hide resolved

yongman approved these changes Jun 6, 2024

View reviewed changes

extract common func

616a245

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot bot added the dco-signoff: yes Indicates the PR's author has signed the dco. label Jun 6, 2024

HuSharp requested a review from JmPotato June 6, 2024 06:02

okJiang reviewed Jun 6, 2024

View reviewed changes

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024

address comment

509c5fb

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the support_scan_regions_hex branch from 44aaf74 to 509c5fb Compare June 6, 2024 10:02

make static happy

ea750a2

Signed-off-by: husharp <jinhao.hu@pingcap.com>

okJiang approved these changes Jun 7, 2024

View reviewed changes

nolouch approved these changes Jun 13, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 13, 2024

rleungx reviewed Jun 13, 2024

View reviewed changes

change function to bytes and add more comment

3f515a6

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the support_scan_regions_hex branch from 7e69cb1 to 3f515a6 Compare June 13, 2024 06:16

add invalid key test

9377fd5

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp requested review from okJiang and rleungx June 13, 2024 06:29

JmPotato approved these changes Jun 13, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 13, 2024

ti-chi-bot bot added 2 commits June 13, 2024 06:40

Merge branch 'master' into support_scan_regions_hex

7ec5d7c

Merge branch 'master' into support_scan_regions_hex

364b209

ti-chi-bot bot merged commit 14c68f6 into tikv:master Jun 13, 2024
20 checks passed

HuSharp deleted the support_scan_regions_hex branch June 13, 2024 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: supports `/regions/key` by hex key #8262

api: supports `/regions/key` by hex key #8262

HuSharp commented Jun 6, 2024

ti-chi-bot bot commented Jun 6, 2024

HuSharp commented Jun 6, 2024

codecov bot commented Jun 6, 2024 •

edited

Loading

JmPotato left a comment

ti-chi-bot bot commented Jun 6, 2024

HuSharp commented Jun 6, 2024

okJiang left a comment

okJiang Jun 6, 2024

HuSharp Jun 6, 2024

okJiang Jun 6, 2024 •

edited

Loading

HuSharp Jun 6, 2024

HuSharp Jun 6, 2024

JmPotato Jun 6, 2024

okJiang Jun 7, 2024

HuSharp Jun 13, 2024 •

edited

Loading

ti-chi-bot bot commented Jun 7, 2024

rleungx Jun 13, 2024

HuSharp Jun 13, 2024 •

edited

Loading

rleungx Jun 13, 2024

ti-chi-bot bot commented Jun 13, 2024

ti-chi-bot bot commented Jun 13, 2024

	func ParseHexKeys(format string, keys ...*string) error {
	func ParseHexKeys(format string, keys []string) ([]string, error) {

api: supports /regions/key by hex key #8262

api: supports /regions/key by hex key #8262

Conversation

HuSharp commented Jun 6, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jun 6, 2024

HuSharp commented Jun 6, 2024

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

JmPotato left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 6, 2024

HuSharp commented Jun 6, 2024

okJiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 7, 2024

Choose a reason for hiding this comment

HuSharp Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 13, 2024

ti-chi-bot bot commented Jun 13, 2024

[LGTM Timeline notifier]

api: supports `/regions/key` by hex key #8262

api: supports `/regions/key` by hex key #8262

codecov bot commented Jun 6, 2024 •

edited

Loading

okJiang Jun 6, 2024 •

edited

Loading

HuSharp Jun 13, 2024 •

edited

Loading

HuSharp Jun 13, 2024 •

edited

Loading