Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

tracker: get some session variables from downtream #1032

Merged
merged 29 commits into from
Sep 21, 2020
Merged

tracker: get some session variables from downtream #1032

merged 29 commits into from
Sep 21, 2020

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Sep 14, 2020 •
edited
Loading

What problem does this PR solve?

enable some test about altering PK, and reduce user config. close #1003

What is changed and how it works?

  • session variable:
    • if user specified, use it
    • else get sql_mode, tidb_skip_utf8_check from downstream
  • tracker (tidb) config:
    • set alter-primary-key true
  • max-allow-packet
    • always get from downstream (by feature of driver)

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has persistent data change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@lance6716 lance6716 added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/feature New feature labels Sep 14, 2020
@lance6716
Copy link
Collaborator Author

/run-all-tests

lance6716
lance6716 commented Sep 15, 2020
View reviewed changes
tests/sharding/run.sh
@@ -18,6 +18,7 @@ EOF

function run() {
run_sql_both_source "SET @@GLOBAL.SQL_MODE='NO_ZERO_IN_DATE,NO_ZERO_DATE'"
run_sql_tidb "SET @@GLOBAL.SQL_MODE='NO_ZERO_IN_DATE,NO_ZERO_DATE'"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this test include a sql file that execute

alter table t2 add column create_by DATETIME NOT NULL DEFAULT '0000-00-00 00:00:00';

@lance6716 lance6716 added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label Sep 15, 2020
csuzhangxc
csuzhangxc reviewed Sep 16, 2020
View reviewed changes
pkg/schema/tracker.go Show resolved Hide resolved
pkg/schema/tracker.go Show resolved Hide resolved
@csuzhangxc
Copy link
Member

need update docs & release notes, right?

@lance6716 lance6716 added needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Sep 16, 2020
@lance6716 lance6716 changed the title tracker: get alter-pk and some session variables from downtream tracker: get some session variables from downtream Sep 16, 2020
@csuzhangxc
Copy link
Member

CI fail

GMHDBJD
GMHDBJD reviewed Sep 21, 2020
View reviewed changes
pkg/schema/tracker.go
// we might SetGlobalConfig before every call to tracker, or use some patch like https://github.com/bouk/monkey
toSet := tidbConfig.NewConfig()
toSet.AlterPrimaryKey = true
tidbConfig.StoreGlobalConfig(toSet)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any test for it? rest LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in unit test
https://github.com/pingcap/dm/pull/1032/files#diff-16690a9b535181de13c90f2f8ded85f9R144

csuzhangxc
csuzhangxc reviewed Sep 21, 2020
View reviewed changes
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Sep 21, 2020
@csuzhangxc csuzhangxc added this to the v2.0.0 RC.3 milestone Sep 21, 2020
GMHDBJD
GMHDBJD approved these changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

LGTM

@GMHDBJD GMHDBJD added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Sep 21, 2020
@lance6716 lance6716 merged commit 0dbc60e into pingcap:master Sep 21, 2020
@lance6716 lance6716 deleted the retrieve-tidb branch September 21, 2020 06:40
@ti-srebot
Copy link

cherry pick to release-2.0 failed

lance6716 added a commit to lance6716/dm that referenced this pull request Sep 21, 2020
@lance6716
tracker: get some session variables from downtream (pingcap#1032)
cdd4c8e
@lance6716 lance6716 mentioned this pull request Sep 21, 2020
Closed
lance6716 added a commit to lance6716/dm that referenced this pull request Sep 21, 2020
@lance6716
tracker: get some session variables from downtream (pingcap#1032)
d797cf7
@lance6716 lance6716 mentioned this pull request Sep 21, 2020
Merged
@lance6716 lance6716 added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Sep 21, 2020
csuzhangxc pushed a commit that referenced this pull request Sep 22, 2020
@lance6716
tracker: get some session variables from downtream (#1032) (#1071)
7508475
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Oct 27, 2020
@lance6716 lance6716 mentioned this pull request Oct 27, 2020
Merged
7 tasks
@csuzhangxc csuzhangxc added already-update-docs The docs related to this PR already updated. Add this label once the docs are updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Oct 27, 2020
@ti-srebot ti-srebot mentioned this pull request Oct 27, 2020
Merged
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@csuzhangxc csuzhangxc csuzhangxc left review comments

@GMHDBJD GMHDBJD GMHDBJD approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked already-update-docs The docs related to this PR already updated. Add this label once the docs are updated already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

auto discover more config, such as max-allowed-packet
4 participants
@lance6716 @csuzhangxc @ti-srebot @GMHDBJD