From 3e0fbbdbe08e0ef86fa8b2cd56e88bdcec2ee9e3 Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 15:57:19 +0800 Subject: [PATCH 01/10] br: fix compatibility issue with concurrent ddl --- br/pkg/conn/conn.go | 7 +++++++ br/pkg/version/version.go | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/br/pkg/conn/conn.go b/br/pkg/conn/conn.go index cd96e8f68a992..98e99e15a90e6 100755 --- a/br/pkg/conn/conn.go +++ b/br/pkg/conn/conn.go @@ -241,6 +241,13 @@ func NewMgr( if err != nil { return nil, errors.Trace(err) } + // we must check tidb(tikv version) any time after concurrent ddl feature implemented in v6.2. + // when tidb < 6.2 we need set EnableConcurrentDDL false to make ddl works. + // we will keep this check until 7.0, which allow the breaking changes. + err = version.CheckClusterVersion(ctx, controller.GetPDClient(), version.CheckVersionForDDL) + if err != nil { + return nil, errors.Annotate(err, "") + } } mgr := &Mgr{ diff --git a/br/pkg/version/version.go b/br/pkg/version/version.go index 2a818a6ee4f85..88c8c3b2ba988 100644 --- a/br/pkg/version/version.go +++ b/br/pkg/version/version.go @@ -18,6 +18,7 @@ import ( "github.com/pingcap/tidb/br/pkg/logutil" "github.com/pingcap/tidb/br/pkg/utils" "github.com/pingcap/tidb/br/pkg/version/build" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/util/engine" pd "github.com/tikv/pd/client" "go.uber.org/zap" @@ -151,6 +152,16 @@ func CheckVersionForBRPiTR(s *metapb.Store, tikvVersion *semver.Version) error { return nil } +// CheckVersionForDDL checks whether we use queue or table to execute ddl during restore. +func CheckVersionForDDL(s *metapb.Store, tikvVersion *semver.Version) error { + // use tikvVersion instead of tidbVersion since br doesn't have mysql client to connect tidb. + if tikvVersion.Major < 6 || (tikvVersion.Major == 6 && tikvVersion.Minor <= 1) { + variable.EnableConcurrentDDL.Store(false) + return nil + } + return nil +} + // CheckVersionForBR checks whether version of the cluster and BR itself is compatible. func CheckVersionForBR(s *metapb.Store, tikvVersion *semver.Version) error { BRVersion, err := semver.NewVersion(removeVAndHash(build.ReleaseVersion)) From 3b6518d23019af65bf681da547a80fe23b4b127e Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 16:01:44 +0800 Subject: [PATCH 02/10] add test --- br/pkg/version/version_test.go | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/br/pkg/version/version_test.go b/br/pkg/version/version_test.go index 008e8a10d1ccd..c086375b2118e 100644 --- a/br/pkg/version/version_test.go +++ b/br/pkg/version/version_test.go @@ -13,6 +13,7 @@ import ( "github.com/coreos/go-semver/semver" "github.com/pingcap/kvproto/pkg/metapb" "github.com/pingcap/tidb/br/pkg/version/build" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/stretchr/testify/require" pd "github.com/tikv/pd/client" ) @@ -261,6 +262,41 @@ func TestCheckClusterVersion(t *testing.T) { require.Error(t, err) } + { + mock.getAllStores = func() []*metapb.Store { + return []*metapb.Store{{Version: "v6.4.0"}} + } + err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) + require.NoError(t, err) + require.True(t, variable.EnableConcurrentDDL.Load()) + } + + { + mock.getAllStores = func() []*metapb.Store { + return []*metapb.Store{{Version: "v6.2.0"}} + } + err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) + require.NoError(t, err) + require.True(t, variable.EnableConcurrentDDL.Load()) + } + + { + mock.getAllStores = func() []*metapb.Store { + return []*metapb.Store{{Version: "v6.1.0"}} + } + err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) + require.NoError(t, err) + require.False(t, variable.EnableConcurrentDDL.Load()) + } + + { + mock.getAllStores = func() []*metapb.Store { + return []*metapb.Store{{Version: "v5.4.0"}} + } + err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) + require.NoError(t, err) + require.False(t, variable.EnableConcurrentDDL.Load()) + } } func TestCompareVersion(t *testing.T) { From dcb8edb74ee96769d3a29103c1a76ebc28e4fa6c Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 16:06:26 +0800 Subject: [PATCH 03/10] add log --- br/pkg/conn/conn.go | 1 + br/pkg/version/version.go | 1 + 2 files changed, 2 insertions(+) diff --git a/br/pkg/conn/conn.go b/br/pkg/conn/conn.go index 98e99e15a90e6..75b092100fe09 100755 --- a/br/pkg/conn/conn.go +++ b/br/pkg/conn/conn.go @@ -244,6 +244,7 @@ func NewMgr( // we must check tidb(tikv version) any time after concurrent ddl feature implemented in v6.2. // when tidb < 6.2 we need set EnableConcurrentDDL false to make ddl works. // we will keep this check until 7.0, which allow the breaking changes. + // NOTE: must call it after domain created! err = version.CheckClusterVersion(ctx, controller.GetPDClient(), version.CheckVersionForDDL) if err != nil { return nil, errors.Annotate(err, "") diff --git a/br/pkg/version/version.go b/br/pkg/version/version.go index 88c8c3b2ba988..d3b0414fcd46c 100644 --- a/br/pkg/version/version.go +++ b/br/pkg/version/version.go @@ -156,6 +156,7 @@ func CheckVersionForBRPiTR(s *metapb.Store, tikvVersion *semver.Version) error { func CheckVersionForDDL(s *metapb.Store, tikvVersion *semver.Version) error { // use tikvVersion instead of tidbVersion since br doesn't have mysql client to connect tidb. if tikvVersion.Major < 6 || (tikvVersion.Major == 6 && tikvVersion.Minor <= 1) { + log.Info("detected the old version of tidb cluster. set enable concurrent ddl to false") variable.EnableConcurrentDDL.Store(false) return nil } From e019170d2446d003fa7ab24f0504b0dedc981716 Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 16:12:44 +0800 Subject: [PATCH 04/10] update --- br/pkg/conn/conn.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/br/pkg/conn/conn.go b/br/pkg/conn/conn.go index 75b092100fe09..c5996c3530b87 100755 --- a/br/pkg/conn/conn.go +++ b/br/pkg/conn/conn.go @@ -245,9 +245,10 @@ func NewMgr( // when tidb < 6.2 we need set EnableConcurrentDDL false to make ddl works. // we will keep this check until 7.0, which allow the breaking changes. // NOTE: must call it after domain created! + // FIXME: remove this check in v7.0 err = version.CheckClusterVersion(ctx, controller.GetPDClient(), version.CheckVersionForDDL) if err != nil { - return nil, errors.Annotate(err, "") + return nil, errors.Annotate(err, "unable to check cluster version for ddl") } } From 70b7191041015da9394e4aad79828ecf6c42643e Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 16:27:19 +0800 Subject: [PATCH 05/10] address comment --- br/pkg/version/version.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/br/pkg/version/version.go b/br/pkg/version/version.go index d3b0414fcd46c..7e2636612a381 100644 --- a/br/pkg/version/version.go +++ b/br/pkg/version/version.go @@ -155,7 +155,8 @@ func CheckVersionForBRPiTR(s *metapb.Store, tikvVersion *semver.Version) error { // CheckVersionForDDL checks whether we use queue or table to execute ddl during restore. func CheckVersionForDDL(s *metapb.Store, tikvVersion *semver.Version) error { // use tikvVersion instead of tidbVersion since br doesn't have mysql client to connect tidb. - if tikvVersion.Major < 6 || (tikvVersion.Major == 6 && tikvVersion.Minor <= 1) { + requireVersion := semver.New("6.2.0") + if tikvVersion.Compare(*requireVersion) < 0 { log.Info("detected the old version of tidb cluster. set enable concurrent ddl to false") variable.EnableConcurrentDDL.Store(false) return nil From b31f62894a918fb9529b4aeb370e00a00f1e62a2 Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 17:03:57 +0800 Subject: [PATCH 06/10] address comment --- br/pkg/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/br/pkg/version/version.go b/br/pkg/version/version.go index 7e2636612a381..692d19f8c4cf9 100644 --- a/br/pkg/version/version.go +++ b/br/pkg/version/version.go @@ -155,7 +155,7 @@ func CheckVersionForBRPiTR(s *metapb.Store, tikvVersion *semver.Version) error { // CheckVersionForDDL checks whether we use queue or table to execute ddl during restore. func CheckVersionForDDL(s *metapb.Store, tikvVersion *semver.Version) error { // use tikvVersion instead of tidbVersion since br doesn't have mysql client to connect tidb. - requireVersion := semver.New("6.2.0") + requireVersion := semver.New("6.2.0-alpha") if tikvVersion.Compare(*requireVersion) < 0 { log.Info("detected the old version of tidb cluster. set enable concurrent ddl to false") variable.EnableConcurrentDDL.Store(false) From 175fd852247864c1747608415c577fb7e874a7a2 Mon Sep 17 00:00:00 2001 From: 3pointer Date: Fri, 22 Jul 2022 17:49:20 +0800 Subject: [PATCH 07/10] upadte test --- br/pkg/version/version_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/br/pkg/version/version_test.go b/br/pkg/version/version_test.go index c086375b2118e..4d0130ea8be8d 100644 --- a/br/pkg/version/version_test.go +++ b/br/pkg/version/version_test.go @@ -280,6 +280,15 @@ func TestCheckClusterVersion(t *testing.T) { require.True(t, variable.EnableConcurrentDDL.Load()) } + { + mock.getAllStores = func() []*metapb.Store { + return []*metapb.Store{{Version: "v6.2.0-alpha"}} + } + err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) + require.NoError(t, err) + require.True(t, variable.EnableConcurrentDDL.Load()) + } + { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v6.1.0"}} From 5c30510ff18a506df5defc2ab1b2bd5cc2f9bb2a Mon Sep 17 00:00:00 2001 From: 3pointer Date: Sat, 23 Jul 2022 14:04:32 +0800 Subject: [PATCH 08/10] fix test --- br/pkg/version/version_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/br/pkg/version/version_test.go b/br/pkg/version/version_test.go index 4d0130ea8be8d..183d292b254d1 100644 --- a/br/pkg/version/version_test.go +++ b/br/pkg/version/version_test.go @@ -266,27 +266,30 @@ func TestCheckClusterVersion(t *testing.T) { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v6.4.0"}} } + originVal := variable.EnableConcurrentDDL.Load() err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) - require.True(t, variable.EnableConcurrentDDL.Load()) + require.Equal(t, originVal, variable.EnableConcurrentDDL.Load()) } { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v6.2.0"}} } + originVal := variable.EnableConcurrentDDL.Load() err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) - require.True(t, variable.EnableConcurrentDDL.Load()) + require.Equal(t, originVal,variable.EnableConcurrentDDL.Load()) } { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v6.2.0-alpha"}} } + originVal := variable.EnableConcurrentDDL.Load() err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) - require.True(t, variable.EnableConcurrentDDL.Load()) + require.Equal(t, originVal,variable.EnableConcurrentDDL.Load()) } { From 0169170bd7e9deaef6b33c2b68d9b07ba40ca279 Mon Sep 17 00:00:00 2001 From: 3pointer Date: Sat, 23 Jul 2022 14:20:34 +0800 Subject: [PATCH 09/10] fmt --- br/pkg/version/version_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/br/pkg/version/version_test.go b/br/pkg/version/version_test.go index 183d292b254d1..9527836fa500b 100644 --- a/br/pkg/version/version_test.go +++ b/br/pkg/version/version_test.go @@ -279,7 +279,7 @@ func TestCheckClusterVersion(t *testing.T) { originVal := variable.EnableConcurrentDDL.Load() err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) - require.Equal(t, originVal,variable.EnableConcurrentDDL.Load()) + require.Equal(t, originVal, variable.EnableConcurrentDDL.Load()) } { @@ -289,13 +289,14 @@ func TestCheckClusterVersion(t *testing.T) { originVal := variable.EnableConcurrentDDL.Load() err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) - require.Equal(t, originVal,variable.EnableConcurrentDDL.Load()) + require.Equal(t, originVal, variable.EnableConcurrentDDL.Load()) } { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v6.1.0"}} } + variable.EnableConcurrentDDL.Store(true) err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) require.False(t, variable.EnableConcurrentDDL.Load()) @@ -305,6 +306,7 @@ func TestCheckClusterVersion(t *testing.T) { mock.getAllStores = func() []*metapb.Store { return []*metapb.Store{{Version: "v5.4.0"}} } + variable.EnableConcurrentDDL.Store(true) err := CheckClusterVersion(context.Background(), &mock, CheckVersionForDDL) require.NoError(t, err) require.False(t, variable.EnableConcurrentDDL.Load()) From 20393e23f7d6fdd531359d5e0363ddf1d60a4c4b Mon Sep 17 00:00:00 2001 From: 3pointer Date: Sat, 23 Jul 2022 16:32:32 +0800 Subject: [PATCH 10/10] fix unstable rawkv test --- br/tests/br_rawkv/run.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/br/tests/br_rawkv/run.sh b/br/tests/br_rawkv/run.sh index b32cca0f8e41f..b8748d3e9fc1a 100755 --- a/br/tests/br_rawkv/run.sh +++ b/br/tests/br_rawkv/run.sh @@ -53,7 +53,7 @@ test_full_rawkv() { checksum_full=$(checksum $check_range_start $check_range_end) # backup current state of key-values # raw backup is not working with range [nil, nil]. TODO: fix it. - run_br --pd $PD_ADDR backup raw -s "local://$BACKUP_FULL" --crypter.method "aes128-ctr" --crypter.key "0123456789abcdef0123456789abcdef" --start $check_range_start --format hex + run_br --pd $PD_ADDR backup raw -s "local://$BACKUP_FULL" --crypter.method "aes128-ctr" --crypter.key "0123456789abcdef0123456789abcdef" --start $check_range_start --end $check_range_end --format hex clean $check_range_start $check_range_end # Ensure the data is deleted @@ -63,7 +63,7 @@ test_full_rawkv() { fail_and_exit fi - run_br --pd $PD_ADDR restore raw -s "local://$BACKUP_FULL" --crypter.method "aes128-ctr" --crypter.key "0123456789abcdef0123456789abcdef" --start $check_range_start --format hex + run_br --pd $PD_ADDR restore raw -s "local://$BACKUP_FULL" --crypter.method "aes128-ctr" --crypter.key "0123456789abcdef0123456789abcdef" --start $check_range_start --end $check_range_end --format hex checksum_new=$(checksum $check_range_start $check_range_end) if [ "$checksum_new" != "$checksum_full" ];then echo "failed to restore"