*: disable async commit in production to make sure the right commit ts of active-active replication#1912
Open
lcwangchao wants to merge 3 commits intotikv:feature/active-active-85from
Open
Conversation
|
[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
the
dco-signoff: yes
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExtends CI triggers to include feature/* branches, updates github.com/tikv/pd/client version across modules, and adds a KVStore runtime toggle to gate active-active commit behavior with corresponding changes in 2PC/1PC decision points and test utilities. Changes
sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant KVStore
end
rect rgba(255,200,200,0.5)
participant Committer
participant PD
end
Client->>KVStore: Commit request
KVStore->>KVStore: check IsActiveActiveCommitSupportDisabled()
alt flag allows active-active path
KVStore->>Committer: attempt async-commit / 1PC flow
Committer->>PD: Get timestamp / PD interactions
Committer->>KVStore: commit
else flag disables active-active path
KVStore->>Committer: use standard 2PC flow
Committer->>PD: Get timestamp / PD interactions
Committer->>KVStore: commit
end
KVStore->>Client: Commit result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
ti-chi-bot
bot
added
the
size/S
lcwangchao
force-pushed
the
disable_async_commit
branch
2 times, most recently
from
6563410 to
3a99d28
Compare
ti-chi-bot
bot
added
size/M
lcwangchao
force-pushed
the
disable_async_commit
branch
3 times, most recently
from
43201b6 to
2aa1a7f
Compare
ti-chi-bot
bot
added
size/L
lcwangchao
force-pushed
the
disable_async_commit
branch
2 times, most recently
from
31fbda2 to
4582dbc
Compare
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
lcwangchao
force-pushed
the
disable_async_commit
branch
2 times, most recently
from
6558ff1 to
10ceb01
Compare
lcwangchao
changed the title
Contributor
Author
|
/retest |
Contributor
Author
|
@coderabbitai review this PR |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tikv/kv.go (1)
125-128: Consider clarifying the comment to reduce confusion.The field name
disableActiveActiveCommitSupportcombined with "default value is false, which disables the async-commit" creates a double-negative that's hard to follow. Consider rephrasing:- // disableActiveActiveCommitSupport indicates whether to disable active-active commit support. - // The default value is false, which disables the async-commit and 1PC to ensure the commit timestamp should - // always follow the constraint by PD settings `tso-unique-index`. + // disableActiveActiveCommitSupport when true, disables the active-active replication constraint, + // enabling async-commit and 1PC for test purposes. When false (default/production), the constraint + // is enforced, disabling async-commit and 1PC to ensure commit timestamps follow the PD `tso-unique-index` setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tikv/kv.go` around lines 125 - 128, The comment for disableActiveActiveCommitSupport is confusing due to a double-negative; rewrite it to clearly state the default and the effect when the field is true or false (e.g., "disableActiveActiveCommitSupport controls whether active-active commit features (async-commit and 1PC) are disabled. Default false: async-commit and 1PC are enabled. When true: async-commit and 1PC are disabled to enforce PD's tso-unique-index constraints."). Update the comment above the disableActiveActiveCommitSupport field in tikv/kv.go to this clearer wording (or equivalent) so readers can immediately understand the default and the behavior for true/false.txnkv/transaction/2pc.go (1)
124-127: Clarify this safety flag before more call sites depend on it.
IsActiveActiveCommitSupportDisabled()reads liketruemeans "feature off", but both gates below only permit async commit/1PC when it returnstrue. That double negative is easy to wire backwards in anotherkvstoreimplementation. A direct name/comment around the real policy (allow legacy async/1PC in tests, etc.) would be much safer.Also applies to: 1511-1513, 1546-1548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@txnkv/transaction/2pc.go` around lines 124 - 127, The method IsActiveActiveCommitSupportDisabled() is named and documented with a double-negative that causes callers to invert logic incorrectly; rename and re-document it to directly express the policy (e.g., AllowAsyncCommitAnd1PC or EnableAsyncCommitAnd1PC) and update all call sites in this file where async commit/1PC are gated (references to IsActiveActiveCommitSupportDisabled() around the async-commit/1PC checks) so that the gate reads naturally (true => allow async-commit/1PC). Ensure the new name and comment state the actual intent (for example: "AllowAsyncCommitAnd1PC indicates tests/legacy environments may permit async-commit and 1PC despite PD tso-unique-index constraints") and keep behavior identical except for the renamed API and inverted boolean logic in callers as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration_tests/2pc_test.go`:
- Around line 101-104: The test is calling the public escape hatch
WithDisableActiveActiveCommitSupportForTest via tikv.NewKVStore; move/remove
this test-only option from the non-test public API: relocate the
WithDisableActiveActiveCommitSupportForTest implementation into a _test or
test-only file (e.g., tikv test utilities) and/or make it unexported, then
update integration_tests/2pc_test.go to use the test-only helper or construct
the store without that option so production code cannot opt out; ensure
references to WithDisableActiveActiveCommitSupportForTest are removed from
non-test code and only present in test files.
---
Nitpick comments:
In `@tikv/kv.go`:
- Around line 125-128: The comment for disableActiveActiveCommitSupport is
confusing due to a double-negative; rewrite it to clearly state the default and
the effect when the field is true or false (e.g.,
"disableActiveActiveCommitSupport controls whether active-active commit features
(async-commit and 1PC) are disabled. Default false: async-commit and 1PC are
enabled. When true: async-commit and 1PC are disabled to enforce PD's
tso-unique-index constraints."). Update the comment above the
disableActiveActiveCommitSupport field in tikv/kv.go to this clearer wording (or
equivalent) so readers can immediately understand the default and the behavior
for true/false.
In `@txnkv/transaction/2pc.go`:
- Around line 124-127: The method IsActiveActiveCommitSupportDisabled() is named
and documented with a double-negative that causes callers to invert logic
incorrectly; rename and re-document it to directly express the policy (e.g.,
AllowAsyncCommitAnd1PC or EnableAsyncCommitAnd1PC) and update all call sites in
this file where async commit/1PC are gated (references to
IsActiveActiveCommitSupportDisabled() around the async-commit/1PC checks) so
that the gate reads naturally (true => allow async-commit/1PC). Ensure the new
name and comment state the actual intent (for example: "AllowAsyncCommitAnd1PC
indicates tests/legacy environments may permit async-commit and 1PC despite PD
tso-unique-index constraints") and keep behavior identical except for the
renamed API and inverted boolean logic in callers as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cc297e2-733b-4298-ac14-277ba414f1ec
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/workflows/integration.yml.github/workflows/test.ymlexamples/gcworker/go.modexamples/rawkv/go.modexamples/txnkv/1pc_txn/go.modexamples/txnkv/async_commit/go.modexamples/txnkv/delete_range/go.modexamples/txnkv/go.modexamples/txnkv/pessimistic_txn/go.modexamples/txnkv/unsafedestoryrange/go.modgo.modintegration_tests/2pc_test.gointegration_tests/go.modintegration_tests/store_test.gointegration_tests/util_test.gotikv/kv.gotikv/test_util.gotxnkv/transaction/2pc.gotxnkv/transaction/test_probe.go
This reverts commit 70d01fd. Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
lcwangchao
force-pushed
the
disable_async_commit
branch
from
10ceb01 to
9d5a3dc
Compare
Contributor
Author
|
@coderabbitai review this PR again |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tikv/kv.go (1)
125-128: Consider renaming to reduce double-negative confusion.The current naming creates a confusing double-negative:
disableActiveActiveCommitSupport = false(default) → async commit/1PC are disableddisableActiveActiveCommitSupport = true(tests) → async commit/1PC are enabledA clearer name like
enableAsyncCommitForTestor inverting toactiveActiveCommitEnabled(defaulttrueto block async commit,falsein tests to allow it) would make the code more readable.The test option
WithDisableActiveActiveCommitSupportForTest()is also confusing since it actually enables async commit for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tikv/kv.go` around lines 125 - 128, The field disableActiveActiveCommitSupport and its setter WithDisableActiveActiveCommitSupportForTest() are causing double-negative confusion; rename the boolean to a positive, clear name (e.g., enableAsyncCommitForTest or activeActiveCommitEnabled) and invert its default/semantics so the identifier directly describes when async-commit/1PC are allowed; update all references to disableActiveActiveCommitSupport (including any checks, comments, and the WithDisableActiveActiveCommitSupportForTest() method name/behavior) to the new name and semantics, and adjust tests that call WithDisableActiveActiveCommitSupportForTest() so they use the new setter and expected boolean value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tikv/test_util.go`:
- Around line 114-115: Fix the typo in the comment above the call to
disableActiveActiveCommitSupport(opt...) by replacing "exists tests" with
"existing tests" so the comment reads: "To make sure some existing tests with
async-commit or 1pc commit pass"; update the comment near the opt assignment to
reflect this corrected wording.
---
Nitpick comments:
In `@tikv/kv.go`:
- Around line 125-128: The field disableActiveActiveCommitSupport and its setter
WithDisableActiveActiveCommitSupportForTest() are causing double-negative
confusion; rename the boolean to a positive, clear name (e.g.,
enableAsyncCommitForTest or activeActiveCommitEnabled) and invert its
default/semantics so the identifier directly describes when async-commit/1PC are
allowed; update all references to disableActiveActiveCommitSupport (including
any checks, comments, and the WithDisableActiveActiveCommitSupportForTest()
method name/behavior) to the new name and semantics, and adjust tests that call
WithDisableActiveActiveCommitSupportForTest() so they use the new setter and
expected boolean value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0fb0623-da7b-4faf-b710-cf555fffa3b5
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/workflows/integration.yml.github/workflows/test.ymlexamples/gcworker/go.modexamples/rawkv/go.modexamples/txnkv/1pc_txn/go.modexamples/txnkv/async_commit/go.modexamples/txnkv/delete_range/go.modexamples/txnkv/go.modexamples/txnkv/pessimistic_txn/go.modexamples/txnkv/unsafedestoryrange/go.modgo.modintegration_tests/2pc_test.gointegration_tests/go.modintegration_tests/store_test.gointegration_tests/util_test.gotikv/kv.gotikv/test_util.gotxnkv/transaction/2pc.gotxnkv/transaction/test_probe.go
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Contributor
Author
|
/hold |
ti-chi-bot
bot
added
the
do-not-merge/hold
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
1 participant
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.
Summary
ref: pingcap/tidb#64281
This PR makes
client-gosafer for active-active replication in production, while keeping the existing async commit / 1PC test coverage intact.Revert the PD dependency change in
go.modso the behavior stays compatible with TiDB/TiKV 8.5.This is aligned with the discussion in PD issue #10427, where PD is moving toward ensuring TSO uniqueness across clusters via suffix bits rather than the previous local/global TSO behavior.
Force disable async commit and 1PC in production.
The reason is that in active-active scenarios, async commit and 1PC may derive commit TS from
maxReadTS + 1instead of getting it directly from PD, which can violate the PDtso-unique-indexconstraint and potentially produce invalid commit TS behavior across clusters.Keep async commit and 1PC available in test environments.
Existing async commit / 1PC tests still need to verify those code paths, so this PR adds a test-only switch to re-enable them when building test stores.
Add coverage for the default behavior.
This PR adds tests to verify that by default async commit and 1PC are disabled, even if a transaction explicitly calls
SetEnableAsyncCommit(true)orSetEnable1PC(true).Summary by CodeRabbit
Chores
New Features
Tests