enhance: [3.0] enable TiKV 1PC by default (#51915)

Project: milvus-io/milvus | Language: Go | Concepts: configuration-management | Difficulty: junior
Link: https://github.com/milvus-io/milvus/pull/52137

The problem

The TiKV client in Milvus was not enabling the "1PC" (one-phase commit) feature by default. This meant distributed transactions would always use the slower two-phase commit unless explicitly configured otherwise. The code also did not clearly separate configuration logic, making it harder to test and maintain.

Why it's non-trivial

A naive fix might simply set the new configuration flags in the client constructor, but this would mix configuration logic with client initialization and make the code harder to test. Additionally, changing commit settings can have subtle side effects if not handled carefully, especially with related flags like async commit.

The approach

1. Refactor configuration logic into a helper function

The PR introduces a new function to encapsulate all TiKV client configuration logic, making it easier to test and maintain:

+func applyTiKVClientConfig(cfg *paramtable.TiKVConfig, conf *config.Config) {
+	if cfg.TiKVUseSSL.GetAsBool() {
+		conf.Security = config.NewSecurity(cfg.TiKVTLSCACert.GetValue(), cfg.TiKVTLSCert.GetValue(), cfg.TiKVTLSKey.GetValue(), []string{})
+	}
+	conf.Enable1PC = true
+	conf.EnableAsyncCommit = false
+}

This function applies the SSL settings if needed, enables 1PC by default, and disables async commit. This centralizes all related configuration in one place.

2. Update client initialization to use the new configuration function

The client creation function now updates global config using the helper, then creates the client:

func GetTiKVClient(cfg *paramtable.TiKVConfig) (*txnkv.Client, error) {
+	config.UpdateGlobal(func(conf *config.Config) {
+		applyTiKVClientConfig(cfg, conf)
+	})
+
+	return txnkv.NewClient([]string{cfg.Endpoints.GetValue()})
+}

This ensures that every new client gets the correct configuration before it is created.

3. Add a unit test for the configuration logic

A new test checks that the helper function sets the correct flags:

+func TestApplyTiKVClientConfigEnable1PC(t *testing.T) {
+	cfg := &paramtable.TiKVConfig{}
+	cfg.Init(paramtable.NewBaseTable())
+
+	conf := config.DefaultConfig()
+	conf.EnableAsyncCommit = true
+
+	applyTiKVClientConfig(cfg, &conf)
+
+	assert.True(t, conf.Enable1PC)
+	assert.False(t, conf.EnableAsyncCommit)
+}

This test verifies that after applying the config, 1PC is enabled and async commit is disabled, regardless of the initial state.

From the review thread

No notable review discussion on this PR.

Takeaways

• Centralize configuration logic in a dedicated function to improve testability and maintainability.
• Always add targeted unit tests for configuration changes to ensure flags are set as intended.
• When enabling a new default, explicitly set related options (e.g., disabling async commit when enabling 1PC) to avoid subtle side effects.