Skip to content

Conversation

spongedu
Copy link
Contributor

@spongedu spongedu commented Jul 30, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

for #7195

  • Improvement
  • Compatibility

How has this PR been tested? (mandatory)

Unittest

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@spongedu
Copy link
Contributor Author

spongedu commented Aug 9, 2018

@zz-jason PTAL

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

zz-jason commented Aug 9, 2018

/run-all-tests

@zz-jason
Copy link
Member

zz-jason commented Aug 9, 2018

@XuHuaiyu @jackysp PTAL

@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. labels Aug 9, 2018
@zz-jason
Copy link
Member

zz-jason commented Aug 9, 2018

@spongedu seems we should specially handle the string "default".

@zz-jason
Copy link
Member

/run-common-test

@spongedu
Copy link
Contributor Author

@zz-jason We've dealed with value default in ValidateSetSystemVar, but the default may translate into the specific default value in function getVarValue in executor/set.go. The former test fail is due to unreasonable usage of ParseInt on an uint64 default value(18446744073709551615) .

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason
Copy link
Member

@jackysp PTAL

@zz-jason
Copy link
Member

@XuHuaiyu @winoros PTAL

@zhexuany
Copy link
Contributor

/run-all-tests

winoros
winoros previously approved these changes Aug 15, 2018
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@zhexuany zhexuany dismissed winoros’s stale review August 16, 2018 03:09

comments need to be addressed.

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 17, 2018
@zz-jason
Copy link
Member

/run-integration-ddl-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants