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.

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

Already on GitHub? Sign in to your account

*: deny unknown fields when deserializing config TOML #5190

Merged
merged 3 commits into from Aug 9, 2019

Conversation

@kennytm
Copy link
Contributor

commented Aug 4, 2019

What have you changed? (mandatory)

Add #[serde(deny_unknown_fields)] to all config structures, which will cause deserialization to fail if the source contains an unrecognized field. The rationale to catch typo or misplaced settings.

What are the type of the changes? (mandatory)

  • Improvement (change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

cargo test --test integrations -- test_error_on_unrecognized_config

Does this PR affect documentation (docs) or release note? (mandatory)

Release note.

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

Hopefully no 馃檭.

Refer to a related PR or issue link (optional)

pingcap/tidb#9855

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

*: deny unknown fields when deserializing config TOML
Signed-off-by: kennytm <kennytm@gmail.com>
@kennytm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

/run-integration-tests

@siddontang
Copy link
Contributor

left a comment

Great!

LGTM

@5kbpers

5kbpers approved these changes Aug 5, 2019

Copy link
Member

left a comment

LGTM

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Does this PR need to be cherry picked to 3.0?

@kennytm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@breeswish We could. WDYT

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@BusyJay @overvenus Do you have some ideas?

@@ -75,6 +75,7 @@ fn memory_mb_for_cf(is_raft_db: bool, cf: &str) -> usize {

#[derive(Clone, Serialize, Deserialize, PartialEq, Debug)]
#[serde(default)]
#[serde(deny_unknown_fields)]

This comment has been minimized.

Copy link
@overvenus

overvenus Aug 5, 2019

Contributor

Seems we can no longer downgrade if there is a new config exist?

This comment has been minimized.

Copy link
@kennytm

kennytm Aug 5, 2019

Author Contributor

You'd need to comment out that config manually. The error message will tell you the name of the offending field.

This comment has been minimized.

Copy link
@siddontang

siddontang Aug 6, 2019

Contributor

seem TiDB has the same problem now. is it ok?
@shenli

This comment has been minimized.

Copy link
@shenli

shenli Aug 8, 2019

Member

yes, I think so.

@kennytm kennytm added the S: CanMerge label Aug 9, 2019

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

/run-all-tests

@sre-bot sre-bot merged commit 0343796 into tikv:master Aug 9, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Need cherry pick? @siddontang @BusyJay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can鈥檛 perform that action at this time.