Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

br: fix compatibility issue with concurrent ddl #36474

Merged
merged 11 commits into from
Jul 23, 2022
Merged

br: fix compatibility issue with concurrent ddl #36474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e0fbbd
br: fix compatibility issue with concurrent ddl
3pointer Jul 22, 2022
3b6518d
add test
3pointer Jul 22, 2022
dcb8edb
add log
3pointer Jul 22, 2022
e019170
update
3pointer Jul 22, 2022
70b7191
address comment
3pointer Jul 22, 2022
b31f628
address comment
3pointer Jul 22, 2022
175fd85
upadte test
3pointer Jul 22, 2022
5c30510
fix test
3pointer Jul 23, 2022
0169170
fmt
3pointer Jul 23, 2022
20393e2
fix unstable rawkv test
3pointer Jul 23, 2022
3f99fa2
Merge branch 'master' into fix_compatibility_issue_with_ddl
ti-chi-bot Jul 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions br/pkg/conn/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,15 @@ 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.
// 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, "unable to check cluster version for ddl")
}
}

mgr := &Mgr{
Expand Down
13 changes: 13 additions & 0 deletions br/pkg/version/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -151,6 +152,18 @@ 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.
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to set it back?

Copy link
Contributor Author

@3pointer 3pointer Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yes, this variable only works for br when br exists.

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))
Expand Down
50 changes: 50 additions & 0 deletions br/pkg/version/version_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -261,6 +262,55 @@ func TestCheckClusterVersion(t *testing.T) {
require.Error(t, err)
}

{
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.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.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.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())
}

{
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())
}
}

func TestCompareVersion(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions br/tests/br_rawkv/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand Down