server, core: implement the query region gRPC server #8979

JmPotato · 2025-01-08T05:23:46Z

What problem does this PR solve?

Issue Number: ref #8690.

What is changed and how does it work?

Implement the query region gRPC server interface.

Check List

Tests

Unit test
Integration test

Release note

None.

codecov · 2025-01-08T05:40:55Z

Codecov Report

Attention: Patch coverage is 58.50340% with 61 lines in your changes missing coverage. Please review.

Project coverage is 76.25%. Comparing base (604b0d6) to head (54b9646).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8979      +/-   ##
==========================================
- Coverage   76.28%   76.25%   -0.04%     
==========================================
  Files         465      465              
  Lines       70512    70659     +147     
==========================================
+ Hits        53792    53880      +88     
- Misses      13372    13428      +56     
- Partials     3348     3351       +3

Flag	Coverage Δ
unittests	`76.25% <58.50%> (-0.04%)`	⬇️

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

rleungx · 2025-01-09T08:42:15Z

pkg/core/region.go

+	regions := r.getRegionsByKeys(keys)
+	// Assert the returned regions count matches the input keys.
+	if len(regions) != len(keys) {
+		panic("returned regions count mismatch with the input keys")


Shall we return an error instead of panic?

My idea is to set such panics at the initial stage of implementing this feature to help with testing. Once the feature needs to be released, I will change them to regular errors.

rleungx · 2025-01-09T08:54:13Z

pkg/core/region.go

@@ -1464,6 +1464,97 @@ func (r *RegionsInfo) GetStoreRegions(storeID uint64) []*RegionInfo {
 	return regions
 }

+// TODO: benchmark the performance of `QueryRegions`.
+// QueryRegions searches RegionInfo from regionTree by keys and IDs in batch.
+func (r *RegionsInfo) QueryRegions(


Will we pass both keys and prevKeys at the same time?

Yes, if the GetRegion and GetPrevRegion are batched in the same request.

rleungx · 2025-01-13T02:48:53Z

server/grpc_service.go

+
+		// TODO: add forwarding logic.
+
+		if s.IsClosed() {


Do we need to check if cluster is started?

rleungx · 2025-01-13T02:49:37Z

pkg/core/region.go

+			regionsByID[id] = regionResp
+		}
+	}
+	return keyIDMap, prevKeyIDMap, regionsByID


What are those maps used for?

It helps the client map the keys passed in with their corresponding region IDs.

nolouch · 2025-01-13T05:48:07Z

pkg/core/region.go

+func (r *RegionsInfo) getRegionsByKeys(keys [][]byte) []*RegionInfo {
+	r.t.RLock()
+	defer r.t.RUnlock()
+	return r.tree.searchByKeys(keys)


I wonder if having too many keys will affect the lock's performance. we could implement it by dividing it into multiple batch search methods.

Batch parallel search is an optimization within the plan, and I want to implement it after completing an MVP.

we need to prevent the behavior of holding the lock for too long.

nolouch

lgtm

ti-chi-bot · 2025-01-17T07:26:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [nolouch,rleungx]

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 · 2025-01-17T07:26:35Z

[LGTM Timeline notifier]

Timeline:

2025-01-13 05:07:43.313003384 +0000 UTC m=+762206.601835088: ☑️ agreed by rleungx.
2025-01-17 07:26:34.606128569 +0000 UTC m=+276266.061174711: ☑️ agreed by nolouch.

rleungx · 2025-01-17T07:45:19Z

/hold
Wait for updating kvproto.

Signed-off-by: JmPotato <[email protected]>

JmPotato · 2025-01-17T12:42:02Z

/unhold

Signed-off-by: JmPotato <[email protected]>

JmPotato · 2025-01-17T13:12:43Z

/test pull-integration-realcluster-test

JmPotato · 2025-01-17T13:59:08Z

/test pull-integration-realcluster-test

JmPotato requested review from nolouch, lhy1024, okJiang and rleungx January 8, 2025 05:23

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 8, 2025

JmPotato changed the title ~~server core: implement the query region gRPC server interface~~ server core: implement the query region gRPC client Jan 8, 2025

JmPotato changed the title ~~server core: implement the query region gRPC client~~ server core: implement the query region gRPC server Jan 8, 2025

JmPotato changed the title ~~server core: implement the query region gRPC server~~ server, core: implement the query region gRPC server Jan 8, 2025

JmPotato force-pushed the query_region_server branch from d272713 to 16341ff Compare January 8, 2025 05:34

rleungx reviewed Jan 9, 2025

View reviewed changes

rleungx reviewed Jan 13, 2025

View reviewed changes

server/grpc_service.go Outdated

// TODO: add forwarding logic.

if s.IsClosed() {

Copy link

Member

rleungx Jan 13, 2025

Choose a reason for hiding this comment

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

Do we need to check if cluster is started?

rleungx reviewed Jan 13, 2025

View reviewed changes

rleungx approved these changes Jan 13, 2025

View reviewed changes

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

nolouch reviewed Jan 13, 2025

View reviewed changes

JmPotato force-pushed the query_region_server branch 3 times, most recently from 1118cec to 0f00072 Compare January 17, 2025 04:20

nolouch approved these changes Jan 17, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 17, 2025

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2025

Implement the query region gRPC server interface

287a1a3

Signed-off-by: JmPotato <[email protected]>

Split the keys to search

665cbfa

Signed-off-by: JmPotato <[email protected]>

JmPotato force-pushed the query_region_server branch from 4ed9deb to f50d09d Compare January 17, 2025 12:40

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 17, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2025

Update the kvproto

54b9646

Signed-off-by: JmPotato <[email protected]>

JmPotato force-pushed the query_region_server branch from f50d09d to 54b9646 Compare January 17, 2025 12:46

ti-chi-bot bot merged commit a69ee01 into tikv:master Jan 17, 2025
23 of 25 checks passed

JmPotato deleted the query_region_server branch January 17, 2025 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server, core: implement the query region gRPC server #8979

server, core: implement the query region gRPC server #8979

JmPotato commented Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

rleungx Jan 9, 2025

JmPotato Jan 13, 2025

rleungx Jan 9, 2025

JmPotato Jan 13, 2025

rleungx Jan 13, 2025

rleungx Jan 13, 2025

JmPotato Jan 13, 2025

nolouch Jan 13, 2025 •

edited

Loading

JmPotato Jan 13, 2025

nolouch Jan 13, 2025

nolouch left a comment

ti-chi-bot bot commented Jan 17, 2025

ti-chi-bot bot commented Jan 17, 2025

rleungx commented Jan 17, 2025

JmPotato commented Jan 17, 2025

JmPotato commented Jan 17, 2025

JmPotato commented Jan 17, 2025

server, core: implement the query region gRPC server #8979

server, core: implement the query region gRPC server #8979

Conversation

JmPotato commented Jan 8, 2025

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 17, 2025

ti-chi-bot bot commented Jan 17, 2025

[LGTM Timeline notifier]

rleungx commented Jan 17, 2025

JmPotato commented Jan 17, 2025

JmPotato commented Jan 17, 2025

JmPotato commented Jan 17, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

nolouch Jan 13, 2025 •

edited

Loading