api: support to query whether pd has loaded region #8749

lhy1024 · 2024-10-28T09:04:44Z

What problem does this PR solve?

Issue Number: Close #8748, close #8426

What is changed and how does it work?

Check List

Tests

Unit test
Integration test

Release note

None.

Signed-off-by: lhy1024 <admin@liudos.us>

codecov · 2024-10-28T09:11:58Z

Codecov Report

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

Project coverage is 76.22%. Comparing base (c5812ee) to head (dd79eb6).
Report is 150 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8749      +/-   ##
==========================================
- Coverage   76.26%   76.22%   -0.04%     
==========================================
  Files         461      462       +1     
  Lines       70392    70429      +37     
==========================================
+ Hits        53682    53687       +5     
- Misses      13356    13383      +27     
- Partials     3354     3359       +5

Flag	Coverage Δ
unittests	`76.22% <93.75%> (-0.04%)`	⬇️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rleungx · 2024-10-29T08:55:54Z

pkg/storage/storage.go

+	regionLoadedFromDefault bool
+	regionLoadedFromStorage bool


Why do we need to distinguish these two kinds?

I want to avoid problems after ps.useRegionStorage switch

How about using string?

regionLoadedFrom: "", "etcd", "leveldb"

I don't think so, we just have these two ways now

I prefer just using a single field to store this info like:

type regionSource int const ( unloaded regionSource = iota etcd leveldb )

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2024-10-30T04:24:24Z

server/api/status.go

@@ -39,11 +40,13 @@ func newStatusHandler(svr *server.Server, rd *render.Render) *statusHandler {
 // @Success  200  {object}  versioninfo.Status
 // @Router   /status [get]


I am not sure whether it is suitable. ref #8748 (comment)

How about /member or /health?

/members return all member info according etcd.ListEtcdMembers
/health is to query the status of each node through the /ping interface to return information about all the nodes, if we don't add a new api, we won't be able to get the result of whether the load region is complete or not.

status is more like some fixed information to me, at least for the current definition.

put it under /status, which is appropriate from the name, but currently status returns compiled information.

Yes, as I said in the issue.

rleungx · 2024-10-30T07:19:19Z

pkg/versioninfo/versioninfo.go

+	Version          string `json:"version"`
+	GitHash          string `json:"git_hash"`
+	StartTimestamp   int64  `json:"start_timestamp"`
+	AreRegionsLoaded bool   `json:"are_regions_loaded"`


prefer to use loaded

Loaded bool json:"loaded"

?

Signed-off-by: lhy1024 <admin@liudos.us>

okJiang · 2024-11-06T03:55:19Z

pkg/versioninfo/versioninfo.go

@@ -31,6 +31,7 @@ type Status struct {
 	Version        string `json:"version"`
 	GitHash        string `json:"git_hash"`
 	StartTimestamp int64  `json:"start_timestamp"`
+	Loaded         bool   `json:"loaded"`


Prefer RegionLoaded

okJiang · 2024-11-06T06:39:40Z

pkg/storage/storage.go

+	regionLoadedFromDefault bool
+	regionLoadedFromStorage bool


How about using string?

regionLoadedFrom: "", "etcd", "leveldb"

okJiang · 2024-11-06T06:43:53Z

pkg/storage/storage.go

 	if atomic.LoadInt32(&ps.useRegionStorage) == 0 {
-		return ps.Storage.LoadRegions(ctx, f)
+		err := ps.Storage.LoadRegions(ctx, f)
+		if err == nil {
+			ps.regionLoadedFromDefault = true
+		}
+		return err
 	}

-	ps.mu.Lock()
-	defer ps.mu.Unlock()
-	if !ps.regionLoaded {
+	if !ps.regionLoadedFromStorage {
 		if err := ps.regionStorage.LoadRegions(ctx, f); err != nil {
 			return err
 		}
-		ps.regionLoaded = true
+		ps.regionLoadedFromStorage = true
 	}
 	return nil


Seems we can use *coreStorage.LoadRegions() directly?

It doesn't check ps.regionLoadedFromStorage

Signed-off-by: lhy1024 <admin@liudos.us>

niubell · 2024-11-12T06:24:08Z

pkg/storage/storage.go

-	useRegionStorage int32
-	regionLoaded     bool
-	mu               syncutil.Mutex
+	useRegionStorage        int32


Is that safe to switch between using or not using RegionStore in the master branch or the latest releases now?

defaultUseRegionStorage is true and only check switch when it was just elected leader.

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2024-11-18T05:19:27Z

pkg/storage/storage.go

-	useRegionStorage int32
-	regionLoaded     bool
-	mu               syncutil.Mutex
+	useRegionStorageFlag int32


Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2024-11-18T05:55:29Z

/test pull-integration-realcluster-test

Tema · 2024-11-19T17:45:10Z

server/api/status.go

@@ -44,6 +45,7 @@ func (h *statusHandler) GetPDStatus(w http.ResponseWriter, _ *http.Request) {
 		GitHash:        versioninfo.PDGitHash,
 		Version:        versioninfo.PDReleaseVersion,
 		StartTimestamp: h.svr.StartTimestamp(),
+		RegionLoaded:   storage.AreRegionsLoaded(h.svr.GetStorage()),


Can we have a more general field here like "Ready" which we can extend in the future if we find more conditions under which PD can't serve?

Does PDStatus API exposed by all components of disaggregated PD?

Any chance we introduce a new API /ready which we can use across all TiDB components in the future?

Hi Tema, thanks for your advice
Q1: A general field will be helpful to expand different scenarios, but we found that currently there is only RegionLoaded is needed. If there are other situations in the future, we tend to add new fields with more accurate meanings.
Q2: Standalone PD or Scheduling Service will expose this API(especially the RegionLoaded field), others such as TSO Service don't.
Q3: IMO, it's good we can use the same interface to query service status, but the fields may be different. The benefits of unification may not be obvious, but it is indeed a better way.

Generally you want to decouple data plane (pd) from control plane (tidb-operator, tiUp), so that if you make changes to one, you don't have to upgrade other and it can start leveraging any improvements right away. That is why usually the common convention is to use /ready across all components. This way the control-plane can have a common logic for rolling restart of all components, without need to necessarily overcustomize each of them. This is not just about PD microservices but many other components of TiDB cluster (tidb, dm, cdc, tikv). Some of them still require additional customization, but it would be nice to keep it to the minimum. Did you talk to tidb-operator team? Don't they want to have a /ready across all components. I've herd there is a work on tidb-operator v2 or something like that. This might be a good opportunity to standartize protocol between control plane and data place as once it is out there, it is hard to change.

PTAL @csuzhangxc

I agree with Tema, for TiDB Operator, it's better to use a general API (like /ready) to check the status in most cases. If this API can't meet some other special cases in the future, then we can consider to add/use some other APIs.

For this RegionLoaded case, in fact, TiDB Operator just wants to know when it can restart the next instance safely.

@niubell can we land this one and open a separate task for a more general /ready endpoint?

@niubell can we land this one and open a separate task for a more general /ready endpoint?
@Tema Agreed, we will merge this PR first. After the general /ready proposal for all components is finalized, PD will also add the /ready API, cc @zhangjinpeng87 @csuzhangxc

lhy1024 · 2024-12-09T04:14:03Z

@rleungx @JmPotato @okJiang PTAL

lhy1024 · 2024-12-09T06:52:05Z

/hold wait for @zhangjinpeng87

rleungx · 2024-12-17T09:06:24Z

pkg/versioninfo/versioninfo.go

@@ -31,6 +31,7 @@ type Status struct {
 	Version        string `json:"version"`
 	GitHash        string `json:"git_hash"`
 	StartTimestamp int64  `json:"start_timestamp"`
+	RegionLoaded   bool   `json:"region_loaded"`


What if the operator/tiup uses this API and PD doesn't provide this field? Will it block the upgrade process?

I don't believe this part is implemented yet, but it should be backward compatible and pass readiness check if it is not provided.

After discussion, we will use /ready api directly.

lhy1024 · 2024-12-18T05:20:21Z

PTAL @rleungx @okJiang

server/api/router.go

Signed-off-by: lhy1024 <admin@liudos.us>

server/apiv2/router.go

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2024-12-18T06:57:44Z

[LGTM Timeline notifier]

Timeline:

2024-12-13 07:08:04.636906218 +0000 UTC m=+595074.725708756: ☑️ agreed by okJiang.
2024-12-18 06:57:43.782319926 +0000 UTC m=+1026453.871122464: ☑️ agreed by rleungx.

rleungx · 2024-12-18T06:58:05Z

/hold

rleungx · 2024-12-18T06:59:37Z

Let's wait for @okJiang to give an approve.

ti-chi-bot · 2024-12-18T09:21:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: okJiang, 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~~ [rleungx]

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

rleungx · 2024-12-19T05:45:10Z

/hold cancel

ti-chi-bot · 2025-05-13T06:45:04Z

In response to a cherrypick label: new pull request created to branch release-8.5: #9319.
But this PR has conflicts, please resolve them!

close tikv#8426, close tikv#8748 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close #8426, close #8748 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: lhy1024 <admin@liudos.us> Signed-off-by: artem_danilov <artem.danilov@airbnb.com> Co-authored-by: lhy1024 <liuhanyang@pingcap.com> Co-authored-by: lhy1024 <admin@liudos.us> Co-authored-by: Artem Danilov <artem.danilov@airbnb.com>

api: support to query whether pd has loaded region

c20a4f9

Signed-off-by: lhy1024 <admin@liudos.us>

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2024

rleungx reviewed Oct 29, 2024

View reviewed changes

fix lint

db574ec

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 commented Oct 30, 2024

View reviewed changes

rleungx reviewed Oct 30, 2024

View reviewed changes

lhy1024 added 2 commits November 1, 2024 11:10

address comments

64f12cc

Signed-off-by: lhy1024 <admin@liudos.us>

address comments

0c50bef

Signed-off-by: lhy1024 <admin@liudos.us>

okJiang reviewed Nov 6, 2024

View reviewed changes

lhy1024 added 2 commits November 11, 2024 19:28

test compatibility

9c9e931

Signed-off-by: lhy1024 <admin@liudos.us>

address comments

bc7428b

Signed-off-by: lhy1024 <admin@liudos.us>

niubell reviewed Nov 12, 2024

View reviewed changes

refactor

df4ac32

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Nov 18, 2024

View reviewed changes

pkg/storage/storage.go Outdated

useRegionStorage int32

regionLoaded bool

mu syncutil.Mutex

useRegionStorageFlag int32

Copy link

Member

rleungx Nov 18, 2024

Choose a reason for hiding this comment

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

use bool?

address comments

d0d6875

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 requested review from rleungx, okJiang and JmPotato November 19, 2024 02:29

Tema reviewed Nov 19, 2024

View reviewed changes

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 Dec 9, 2024

lhy1024 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2024

okJiang approved these changes Dec 13, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 13, 2024

rleungx reviewed Dec 17, 2024

View reviewed changes

lhy1024 requested review from okJiang and rleungx December 18, 2024 05:20

rleungx reviewed Dec 18, 2024

View reviewed changes

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

move to api v2

4e7bf41

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Dec 18, 2024

View reviewed changes

server/apiv2/router.go Outdated Show resolved Hide resolved

address comments

ae3ea48

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes Dec 18, 2024

View reviewed changes

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

ti-chi-bot bot added the approved label Dec 18, 2024

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 Dec 18, 2024

okJiang approved these changes Dec 18, 2024

View reviewed changes

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 Dec 19, 2024

Merge branch 'master' into loaded

dd79eb6

ti-chi-bot bot merged commit da4b43a into tikv:master Dec 19, 2024
25 checks passed

hbisheng mentioned this pull request Feb 24, 2025

Implement a /ready API to check server readiness post startup tikv/tikv#18244

Closed

liubog2008 mentioned this pull request Apr 11, 2025

Improve liveness probe and readiness probe pingcap/tidb-operator#6145

Open

2 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label May 13, 2025

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request May 13, 2025

This is an automated cherry-pick of tikv#8749

a218ad9

close tikv#8426, close tikv#8748 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 13, 2025

api: support to query whether pd has loaded region (#8749) #9319

Merged

csuzhangxc mentioned this pull request Jun 12, 2025

opt: check PD member ready when upgrading pingcap/tidb-operator#6243

Merged

10 tasks

ti-chi-bot mentioned this pull request Jun 13, 2025

opt: check PD member ready when upgrading (#6243) pingcap/tidb-operator#6247

Merged

10 tasks

		@@ -39,11 +40,13 @@ func newStatusHandler(svr server.Server, rd render.Render) *statusHandler {
		// @Success 200 {object} versioninfo.Status
		// @Router /status [get]

api: support to query whether pd has loaded region #8749

api: support to query whether pd has loaded region #8749

Uh oh!

Conversation

lhy1024 commented Oct 28, 2024 • edited by rleungx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

codecov bot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhy1024 commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhy1024 commented Dec 9, 2024

Uh oh!

lhy1024 commented Dec 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lhy1024 commented Oct 28, 2024 •

edited by rleungx

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading