*: refactor dynamic tso switch#8780
Open
rleungx wants to merge 14 commits into
Open
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
rleungx
force-pushed
the
remove-api-mode
branch
2 times, most recently
from
baece73 to
e252d4c
Compare
ti-chi-bot
Bot
added
size/XL
rleungx
force-pushed
the
remove-api-mode
branch
from
e7daf0e to
b2ff129
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8780 +/- ##
==========================================
- Coverage 76.29% 75.97% -0.32%
==========================================
Files 465 465
Lines 70539 70567 +28
==========================================
- Hits 53816 53616 -200
- Misses 13376 13618 +242
+ Partials 3347 3333 -14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
rleungx
force-pushed
the
remove-api-mode
branch
from
a5e5fa5 to
0197c74
Compare
rleungx
force-pushed
the
remove-api-mode
branch
from
07eb2ef to
d380e36
Compare
okJiang
reviewed
Dec 9, 2024
okJiang
left a comment
okJiang
left a comment
Member
There was a problem hiding this comment.
Please add some description about these changes.
Member
There was a problem hiding this comment.
Suggested change
| log.Info("campaign %PD leader meets error due to txn conflict, another PD/API server may campaign successfully", | |
| log.Info("campaign PD leader meets error due to txn conflict, another PD/API server may campaign successfully", |
rleungx
force-pushed
the
remove-api-mode
branch
from
d380e36 to
dddb935
Compare
rleungx
force-pushed
the
remove-api-mode
branch
from
6163af7 to
bf82504
Compare
lhy1024
reviewed
Jan 8, 2025
Member
There was a problem hiding this comment.
If access pd mode server, will it return 500?
Member
Author
There was a problem hiding this comment.
After this PR, there won't be pd mode anymore.
Member
There was a problem hiding this comment.
If SchedulingFallbackEnabled is false and there is no scheduling server, will we not try to start scheduling service in pd server?
rleungx
force-pushed
the
remove-api-mode
branch
from
1be1b98 to
fede5a6
Compare
ti-chi-bot
Bot
added
size/XXL
rleungx
force-pushed
the
remove-api-mode
branch
from
b00fb6d to
fc2e1bd
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
remove-api-mode
branch
from
00f2840 to
c416d95
Compare
rleungx
force-pushed
the
remove-api-mode
branch
from
fbda990 to
c8506b7
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
remove-api-mode
branch
from
c8506b7 to
a9d441c
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesTSO provider coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cluster.go (1)
455-510: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRange the original cluster map, not the new empty one.
newTestCluster.serversis allocated empty here, so the loop never restarts any server and the function returns a cluster with no servers and no error. Range overcluster.serversinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cluster.go` around lines 455 - 510, The restart logic in restartTestCluster is iterating over the newly allocated empty servers map, so no servers are restarted. Update the loop to range over the existing cluster.servers map, while keeping the rest of the NewTestServer/serverMap/errorMap flow intact, so each original TestServer is destroyed and recreated correctly.
♻️ Duplicate comments (1)
server/cluster/cluster.go (1)
404-417: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly mark scheduling independent after discovering scheduling servers.
When fallback is disabled, Line 406 falls through to the external-service branch even if discovery failed or returned zero servers. That can stop PD scheduling jobs and mark scheduling independent with no scheduling service available.
Proposed fix
func (c *RaftCluster) checkSchedulingService() { servers, err := discovery.Discover(c.etcdClient, constant.SchedulingServiceName) - if c.opt.GetMicroserviceConfig().IsSchedulingFallbackEnabled() && (err != nil || len(servers) == 0) { - c.startSchedulingJobs(c, c.hbstreams) - c.UnsetServiceIndependent(constant.SchedulingServiceName) - } else { - if c.stopSchedulingJobs() || c.coordinator == nil { - c.initCoordinator(c.ctx, c, c.hbstreams) - } - if !c.IsServiceIndependent(constant.SchedulingServiceName) { - c.SetServiceIndependent(constant.SchedulingServiceName) - } + if err != nil || len(servers) == 0 { + if c.opt.GetMicroserviceConfig().IsSchedulingFallbackEnabled() { + c.startSchedulingJobs(c, c.hbstreams) + c.UnsetServiceIndependent(constant.SchedulingServiceName) + return + } + c.stopSchedulingJobs() + c.UnsetServiceIndependent(constant.SchedulingServiceName) + return + } + if c.stopSchedulingJobs() || c.coordinator == nil { + c.initCoordinator(c.ctx, c, c.hbstreams) + } + if !c.IsServiceIndependent(constant.SchedulingServiceName) { + c.SetServiceIndependent(constant.SchedulingServiceName) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/cluster/cluster.go` around lines 404 - 417, The scheduling-service discovery logic in checkSchedulingService should not enter the external-service path unless discovery actually found servers. Update the branching around discovery.Discover and IsSchedulingFallbackEnabled so that failed/empty discovery only falls back when enabled, and otherwise keeps PD scheduling jobs running and does not call SetServiceIndependent for constant.SchedulingServiceName. Use checkSchedulingService, startSchedulingJobs, stopSchedulingJobs, and SetServiceIndependent/UnsetServiceIndependent to keep the service-independent state consistent with discovery results.
🧹 Nitpick comments (1)
tests/integrations/mcs/tso/proxy_test.go (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: says "1 server" but creates 3.
The comment doesn't match the
NewTestCluster(s.ctx, 3, ...)call.✏️ Fix comment
- // Create an PD cluster with 1 server + // Create a PD cluster with 3 servers s.pdCluster, err = tests.NewTestCluster(s.ctx, 3, func(conf *config.Config, _ string) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/mcs/tso/proxy_test.go` around lines 65 - 68, The inline comment above NewTestCluster is stale and contradicts the actual cluster size being created. Update or remove the “1 server” comment so it matches the tests.NewTestCluster(s.ctx, 3, ...) call and the surrounding setup in proxy_test.go, keeping the description aligned with the configured PD cluster size.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/utils/apiutil/serverapi/middleware.go`:
- Around line 118-121: The redirect gate in matchMicroserviceRedirectRules
currently combines TSO and Scheduling independence with a shared check, which
can allow the /admin/reset-ts rule to be evaluated under the wrong service
condition. Split the guard in redirector.matchMicroserviceRedirectRules so TSO
redirects are checked only when constant.TSOServiceName is independent, and
scheduling redirects are checked separately for constant.SchedulingServiceName;
keep the rule matching logic aligned with the redirect registration in
server/api/server.go.
In `@server/config/config.go`:
- Line 847: The EnableMultiTimelines field in the config struct is missing
explicit TOML/JSON tags, so add matching enable-multi-timelines tags consistent
with the adjacent fields in config.go. Update the config struct definition that
includes EnableMultiTimelines to use explicit json and toml tags, following the
existing naming pattern and adding omitempty where appropriate so the field
binds and serializes correctly.
In `@server/server.go`:
- Around line 1375-1397: The non-TSO path in IsServiceIndependent can
dereference s.cluster before startServer initializes it. Update the method to
guard s.cluster (and the raft cluster state) before calling
cluster.IsServiceIndependent, and return false while the server is not fully
started or the raft cluster is nil/running state is unavailable. Keep the
existing TSO-specific logic intact, but ensure all fallback delegation to
s.cluster only happens after the cluster has been created.
In `@tests/integrations/mcs/tso/server_test.go`:
- Around line 675-678: The assertion message in the TSO service check is stale
and contradicts the current `re.NoError` expectation in `server_test.go`. Update
the failure message in the `checkTSOMonotonic` loop so it matches the intended
behavior that PD should provide TSO service, using the existing `re.NoError`
call and the surrounding TSO check logic as the anchor.
---
Outside diff comments:
In `@tests/cluster.go`:
- Around line 455-510: The restart logic in restartTestCluster is iterating over
the newly allocated empty servers map, so no servers are restarted. Update the
loop to range over the existing cluster.servers map, while keeping the rest of
the NewTestServer/serverMap/errorMap flow intact, so each original TestServer is
destroyed and recreated correctly.
---
Duplicate comments:
In `@server/cluster/cluster.go`:
- Around line 404-417: The scheduling-service discovery logic in
checkSchedulingService should not enter the external-service path unless
discovery actually found servers. Update the branching around discovery.Discover
and IsSchedulingFallbackEnabled so that failed/empty discovery only falls back
when enabled, and otherwise keeps PD scheduling jobs running and does not call
SetServiceIndependent for constant.SchedulingServiceName. Use
checkSchedulingService, startSchedulingJobs, stopSchedulingJobs, and
SetServiceIndependent/UnsetServiceIndependent to keep the service-independent
state consistent with discovery results.
---
Nitpick comments:
In `@tests/integrations/mcs/tso/proxy_test.go`:
- Around line 65-68: The inline comment above NewTestCluster is stale and
contradicts the actual cluster size being created. Update or remove the “1
server” comment so it matches the tests.NewTestCluster(s.ctx, 3, ...) call and
the surrounding setup in proxy_test.go, keeping the description aligned with the
configured PD cluster size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59ddafd6-2c6b-4205-96a0-1be31ed46977
📒 Files selected for processing (55)
cmd/pd-server/main.gopkg/keyspace/keyspace_test.gopkg/mcs/discovery/register.gopkg/mcs/registry/registry.gopkg/storage/endpoint/tso.gopkg/storage/kv/etcd_kv.gopkg/storage/storage_tso_test.gopkg/tso/allocator_manager.gopkg/tso/global_allocator.gopkg/tso/keyspace_group_manager.gopkg/tso/tso.gopkg/utils/apiutil/serverapi/middleware.gopkg/utils/keypath/key_path.goserver/api/member.goserver/api/server_test.goserver/api/version_test.goserver/apiv2/handlers/micro_service.goserver/cluster/cluster.goserver/config/config.goserver/config/service_middleware_config.goserver/config/service_middleware_persist_options.goserver/server.goserver/server_test.goserver/testutil.goserver/util.gotests/cluster.gotests/integrations/client/client_test.gotests/integrations/mcs/discovery/register_test.gotests/integrations/mcs/keyspace/tso_keyspace_group_test.gotests/integrations/mcs/members/member_test.gotests/integrations/mcs/scheduling/api_test.gotests/integrations/mcs/scheduling/config_test.gotests/integrations/mcs/scheduling/meta_test.gotests/integrations/mcs/scheduling/rule_test.gotests/integrations/mcs/scheduling/server_test.gotests/integrations/mcs/tso/api_test.gotests/integrations/mcs/tso/keyspace_group_manager_test.gotests/integrations/mcs/tso/proxy_test.gotests/integrations/mcs/tso/server_test.gotests/integrations/tso/client_test.gotests/integrations/tso/consistency_test.gotests/integrations/tso/server_test.gotests/server/apiv2/handlers/keyspace_test.gotests/server/apiv2/handlers/tso_keyspace_group_test.gotests/server/member/member_test.gotests/server/server_test.gotests/server/tso/tso_test.gotests/testutil.gotools/pd-ctl/pdctl/command/config_command.gotools/pd-ctl/tests/config/config_test.gotools/pd-ctl/tests/global_test.gotools/pd-ctl/tests/keyspace/keyspace_group_test.gotools/pd-ctl/tests/keyspace/keyspace_test.gotools/pd-simulator/main.gotools/pd-simulator/simulator/client.go
💤 Files with no reviewable changes (2)
- tools/pd-simulator/simulator/client.go
- server/api/member.go
| // We should separate the keyspace group from the microservice later. | ||
| EnableTSODynamicSwitching bool `toml:"enable-tso-dynamic-switching" json:"enable-tso-dynamic-switching,string"` | ||
| // TODO: use it to replace system variable. | ||
| EnableMultiTimelines bool |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the enable-multi-timelines config tags.
Line 847 leaves EnableMultiTimelines without TOML/JSON tags, so the documented micro-service.enable-multi-timelines control may not bind from config files and won’t serialize like the adjacent fields.
As per coding guidelines, “Use JSON tags explicitly; use omitempty where sensible.”
Proposed fix
- EnableMultiTimelines bool
+ EnableMultiTimelines bool `toml:"enable-multi-timelines" json:"enable-multi-timelines,string"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| EnableMultiTimelines bool | |
| EnableMultiTimelines bool `toml:"enable-multi-timelines" json:"enable-multi-timelines,string"` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/config/config.go` at line 847, The EnableMultiTimelines field in the
config struct is missing explicit TOML/JSON tags, so add matching
enable-multi-timelines tags consistent with the adjacent fields in config.go.
Update the config struct definition that includes EnableMultiTimelines to use
explicit json and toml tags, following the existing naming pattern and adding
omitempty where appropriate so the field binds and serializes correctly.
Source: Coding guidelines
Comment on lines
+675
to
+678
| // PD should provide TSO service | ||
| for range 10 { | ||
| err = checkTSOMonotonic(ctx, pdClient, &globalLastTS, 1) | ||
| re.Error(err, "TSO service should not be available") | ||
| re.NoError(err, "TSO service should not be available") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale assertion message contradicts inverted check.
The loop was changed to assert re.NoError (i.e., TSO should be available, per the new comment "PD should provide TSO service" on line 675), but the failure message still reads "TSO service should not be available" — a leftover from the previous logic that asserted an error. This will produce a misleading failure message if the assertion ever fails.
🐛 Proposed fix
// PD should provide TSO service
for range 10 {
err = checkTSOMonotonic(ctx, pdClient, &globalLastTS, 1)
- re.NoError(err, "TSO service should not be available")
+ re.NoError(err, "TSO service should be available")
time.Sleep(10 * time.Millisecond)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // PD should provide TSO service | |
| for range 10 { | |
| err = checkTSOMonotonic(ctx, pdClient, &globalLastTS, 1) | |
| re.Error(err, "TSO service should not be available") | |
| re.NoError(err, "TSO service should not be available") | |
| // PD should provide TSO service | |
| for range 10 { | |
| err = checkTSOMonotonic(ctx, pdClient, &globalLastTS, 1) | |
| re.NoError(err, "TSO service should be available") | |
| time.Sleep(10 * time.Millisecond) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrations/mcs/tso/server_test.go` around lines 675 - 678, The
assertion message in the TSO service check is stale and contradicts the current
`re.NoError` expectation in `server_test.go`. Update the failure message in the
`checkTSOMonotonic` loop so it matches the intended behavior that PD should
provide TSO service, using the existing `re.NoError` call and the surrounding
TSO check logic as the anchor.
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/L
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/cluster/cluster_test.go`:
- Around line 2393-2406: The lock contention check in the test is racy because
the goroutine may not have reached c.tsoSwitchMu.Lock() before the negative
assertion runs. In cluster_test.go, update the goroutine around
switchAcquired/releaseSwitch to signal a separate “started attempting lock”
channel immediately before Lock(), then wait for that signal before asserting
the lock was not acquired. Keep the existing switchAcquired and releaseSwitch
flow, but use the new start-attempt signal to prove the provider switch is
actually contending with the embedded TSO request.
In `@server/grpc_service.go`:
- Around line 683-689: Reject zero-count TSO requests in
generateEmbeddedTSOResponse as well, so embedded mode matches the forwarding
path’s validation. Add the same Count == 0 guard before calling
s.generateEmbeddedTSO, returning the appropriate invalid-request error from
generateEmbeddedTSOResponse and keeping request handling consistent across
provider modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eb30988-3500-4a6d-9ac4-b9d1725149cf
📒 Files selected for processing (4)
server/cluster/cluster.goserver/cluster/cluster_test.goserver/forward.goserver/grpc_service.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/grpc_service.go (1)
623-627: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the retry backoff context-aware.
time.Sleepignores stream cancellation while the forwarding slot is held. Return promptly whenctxis canceled. As per coding guidelines, “Use context-aware timeouts and backoff for retries.”Proposed fix
for retry := range maxRetryTimesRequestTSOServer { if retry > 0 { - time.Sleep(retryIntervalRequestTSOServer) + timer := time.NewTimer(retryIntervalRequestTSOServer) + select { + case <-ctx.Done(): + timer.Stop() + return errors.WithStack(ctx.Err()) + case <-timer.C: + } } tsoStreamErr, err = s.handleTSOForwarding(stream.Context(), forwarder, request, tsDeadlineCh)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/grpc_service.go` around lines 623 - 627, The retry loop in handleTSOForwarding/TSO forwarding currently uses a blocking sleep that ignores stream cancellation, so replace the plain backoff wait with a context-aware wait tied to stream.Context() and return immediately if the context is canceled. Keep the retry behavior in the same loop, but ensure the pause between attempts is interruptible by the request context or deadline so the forwarding slot is released promptly on cancel.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/grpc_service.go`:
- Around line 623-627: The retry loop in handleTSOForwarding/TSO forwarding
currently uses a blocking sleep that ignores stream cancellation, so replace the
plain backoff wait with a context-aware wait tied to stream.Context() and return
immediately if the context is canceled. Keep the retry behavior in the same
loop, but ensure the pause between attempts is interruptible by the request
context or deadline so the forwarding slot is released promptly on cancel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c0c4bb5-0321-4688-a088-a6987eb907fc
📒 Files selected for processing (5)
server/cluster/cluster.goserver/cluster/cluster_test.goserver/forward.goserver/grpc_service.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/forward.go
- server/cluster/cluster_test.go
rleungx
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/cluster/cluster.go`:
- Around line 824-842: Move the keyspace-group discovery lookup and default TSO
readiness probe out of the tsoSwitchMu critical section in
switchTSOProviderToTSOIfServiceReady: perform all etcd reads and
isDefaultTSOServiceReady checks before acquiring the lock, then lock only around
the provider-switch mutation while preserving existing readiness and
legacy-primary conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97cd874c-2412-4919-a551-a3b33f189b77
📒 Files selected for processing (24)
client/client.goclient/client_test.goclient/clients/tso/client.goclient/inner_client.goclient/servicediscovery/service_discovery.goclient/servicediscovery/tso_service_discovery.goclient/servicediscovery/tso_service_discovery_test.goerrors.tomlpkg/errs/errno.gopkg/mcs/tso/server/server.gopkg/mcs/utils/util.gopkg/storage/endpoint/tso.gopkg/storage/storage_tso_test.gopkg/tso/allocator.gopkg/tso/tso_test.gopkg/utils/apiutil/serverapi/middleware.gopkg/utils/apiutil/serverapi/middleware_test.goserver/cluster/cluster.goserver/cluster/cluster_test.goserver/forward.goserver/grpc_service.goserver/grpc_service_test.goserver/server.gotests/integrations/mcs/tso/server_test.go
✅ Files skipped from review due to trivial changes (1)
- errors.toml
Comment on lines
+824
to
+842
| func (c *RaftCluster) switchTSOProviderToTSOIfServiceReady(allowLegacyPrimary, probeReady bool) error { | ||
| if c.IsServiceIndependent(constant.TSOServiceName) && !c.isTSOAllocatorInitialized() { | ||
| return nil | ||
| } | ||
| c.tsoSwitchMu.Lock() | ||
| defer c.tsoSwitchMu.Unlock() | ||
| if c.isKeyspaceGroupEnabled { | ||
| tsoServiceState, err := c.getDefaultTSOServiceDiscoveryState() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if tsoServiceState != tsoServiceDiscoveryReady && | ||
| (!allowLegacyPrimary || tsoServiceState != tsoServiceDiscoveryLegacyPrimary) { | ||
| return nil | ||
| } | ||
| } | ||
| if probeReady && !c.isDefaultTSOServiceReady() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '620,860p' server/cluster/cluster.go
printf '\n---\n'
rg -n "tsoSwitchMu|RunEmbeddedTSORequest|switchTSOProviderToPDIfNoTSOServiceLocked|switchTSOProviderToTSOIfServiceReady|getDefaultTSOServiceDiscoveryState|isDefaultTSOServiceReady|tsoServiceReadyCheckTimeout" server/cluster/cluster.goRepository: tikv/pd
Length of output: 10461
🏁 Script executed:
sed -n '532,576p' server/cluster/cluster.goRepository: tikv/pd
Length of output: 2120
🏁 Script executed:
sed -n '590,620p' server/cluster/cluster.go
printf '\n---\n'
rg -n "switchTSOProviderToTSOIfServiceReady\\(" -S .Repository: tikv/pd
Length of output: 1511
Move the discovery/probe out from under tsoSwitchMu
checkTSOService already rechecks discovery before calling this helper, but switchTSOProviderToTSOIfServiceReady still takes the write lock and then does another etcd read and, on the legacy path, a 1s readiness probe. Because RunEmbeddedTSORequest takes tsoSwitchMu.RLock(), that can block embedded TSO requests for the full switch attempt. Read/probe first, then lock only to flip the provider.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/cluster/cluster.go` around lines 824 - 842, Move the keyspace-group
discovery lookup and default TSO readiness probe out of the tsoSwitchMu critical
section in switchTSOProviderToTSOIfServiceReady: perform all etcd reads and
isDefaultTSOServiceReady checks before acquiring the lock, then lock only around
the provider-switch mutation while preserving existing readiness and
legacy-primary conditions.
Keep the current TSO provider unless service discovery confirms that no TSO server is registered. Classify timestamp fence conflicts so embedded PD TSO yields without resigning PD leadership. Signed-off-by: Ryan Leung <rleungx@gmail.com>
Contributor
|
@rleungx: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #8477.
What is changed and how does it work?
This PR made the following changes:
apisubcommand logic to pd. We keep the subcommand for compatibility but won't use it anymore.enable-multi-timelinesto replace it in the next PR.enable-tso-dynamic-switchfor the non-serverless env andenable-multi-timelinesfor the serverless env to manage it.checkTSOServiceandIsServiceIndependentlogic that needs attention when reviewing.Check List
Tests
Release note
Summary by CodeRabbit
New Features
Bug Fixes