Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

config: synchronize actual default value with the toml file #255

Merged
merged 4 commits into from
Feb 4, 2020

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Jan 8, 2020

What problem does this PR solve?

Make the default values when missing a config file to be equivalent to the default TOML file.

Exposed a few settings in command line.

What is changed and how it works?

Changed default values:

  • checkpoint.enable = true
  • post-restore.analyze = true
  • csv.header = true
  • csv.null = '\n'
  • csv.backslash-escape = true

Exposed global / command line options:

  • --no-schema=false
  • --enable-checkpoint=true
  • --analyze=true
  • --checksum=true
  • --check-requirements=true

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    • inspect the config when running bin/tidb-lightning --analyze=true, bin/tidb-lightning --analyze=false, etc.

Side effects

  • Breaking backward compatibility
    • default value with no config value is changed.

Related changes

  • Need to update the documentation

@kennytm kennytm added Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated status/PTAL This PR is ready for review. Add this label back after committing new changes priority/unimportant type/feature New feature labels Jan 8, 2020
@kennytm kennytm force-pushed the kennytm/align-default-config branch from 8b9a9d5 to a3edd5d Compare January 8, 2020 15:56
@kennytm
Copy link
Collaborator Author

kennytm commented Jan 8, 2020

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Jan 8, 2020

PTAL @GregoryIan

@kennytm
Copy link
Collaborator Author

kennytm commented Jan 14, 2020

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Jan 14, 2020

Ping @GregoryIan

@kennytm
Copy link
Collaborator Author

kennytm commented Jan 16, 2020

PTAL @WangXiangUSTC @3pointer

@lance6716
Copy link
Contributor

LGTM (from community)

@kennytm
Copy link
Collaborator Author

kennytm commented Jan 26, 2020

/run-all-tests

(Exposed --check-requirements to command line too.)

@kennytm kennytm added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Feb 4, 2020
@kennytm kennytm merged commit b865826 into master Feb 4, 2020
@kennytm kennytm deleted the kennytm/align-default-config branch February 4, 2020 07:31
@kennytm kennytm removed the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants