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

Jump to bottom

config: deprecate tikv-client.copr-cache.enable and invisible some copr-cache configs #22786

Merged
merged 7 commits into from
Mar 10, 2021
Merged

config: deprecate tikv-client.copr-cache.enable and invisible some copr-cache configs #22786

merged 7 commits into from
Mar 10, 2021

Conversation

lzmhhh123
Copy link
Contributor

Signed-off-by: lzmhhh123 lzmhhh123@gmail.com

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

How it Works: make capcity>0 check whether it enabled.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • remove "enable" field of coprocessor cache configuration.

@lzmhhh123 lzmhhh123 requested a review from a team as a code owner February 18, 2021 07:55
@lzmhhh123 lzmhhh123 requested review from XuHuaiyu and removed request for a team February 18, 2021 07:55
jackysp
jackysp reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

You may need to add it to deprecatedConfig in config.go.

@bb7133
Copy link
Member

bb7133 commented Feb 18, 2021

You can follow this PR #18618 to deprecate this configuration.

@lzmhhh123 lzmhhh123 changed the title config: remove "enable" field of coprocessor cache config: deprecate tikv-client.copr-cache.enable Feb 19, 2021
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2021
jackysp
jackysp reviewed Mar 2, 2021
View reviewed changes
config/config.toml.example
capacity-mb = 1000.0

# Only cache small requests. Zero means no limits for requests.
Copy link
Member

Choose a reason for hiding this comment

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

All of them are deprecated? PTAL @breeswish

breezewish
breezewish reviewed Mar 2, 2021
View reviewed changes
config/config.toml.example
Comment on lines -431 to -433
admission-max-result-mb = 10.0
# Only cache requests takes notable time to process.
admission-min-process-ms = 5
Copy link
Member

Choose a reason for hiding this comment

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

Seems that some users are tend to modify these configurations, do we need to remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a request from the PM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XuHuaiyu PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

@bb7133 may have impression about the client that uses these configuration values. If we drop them, then we cannot serve this kind of clients any more.

Copy link
Member

Choose a reason for hiding this comment

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

How about:

  1. only expose capacity-mb to users by default to increase usability.
  2. make other config items configurable, but hidden by default.

Copy link
Member

Choose a reason for hiding this comment

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

How about:

  1. only expose capacity-mb to users by default to increase usability.
  2. make other config items configurable, but hidden by default.

Hide configurations looks good to me.

@lzmhhh123 lzmhhh123 changed the title config: deprecate tikv-client.copr-cache.enable config: deprecate some configs of tikv-client.copr-cache Mar 2, 2021
@lzmhhh123
config: remove enable and hide some fields for coprocessor cache
8cb08a2 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@lzmhhh123
fix test
38c9c81 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 8, 2021
@lzmhhh123 lzmhhh123 changed the title config: deprecate some configs of tikv-client.copr-cache config: deprecate tikv-client.copr-cache.enable and invisible some copr-cache configs Mar 8, 2021
@lzmhhh123
update comment
6ee8db7 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@lzmhhh123
Copy link
Contributor Author

/cc @XuHuaiyu @breeswish @zz-jason

XuHuaiyu
XuHuaiyu reviewed Mar 9, 2021
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Please add some test results to prove the configurations are invisible for now.

@lzmhhh123
add test
a8ecf0f 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@lzmhhh123 lzmhhh123 mentioned this pull request Mar 10, 2021
Merged
11 tasks
@lzmhhh123
fix fmt
9502f8b 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
XuHuaiyu
XuHuaiyu reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

/LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2021
@lzmhhh123
add
3b76edc 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@XuHuaiyu
Copy link
Contributor

/LGTM

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • XuHuaiyu
  • breeswish

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 10, 2021
@XuHuaiyu
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 3b76edc

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 10, 2021
@lzmhhh123
Copy link
Contributor Author

/merge

@lzmhhh123
Copy link
Contributor Author

unit test failed at:

[2021-03-10T07:06:00.934Z] FAIL: conn_test.go:740: ConnTestSuite.TestTiFlashFallback
[2021-03-10T07:06:00.934Z] 
[2021-03-10T07:06:00.934Z] conn_test.go:785:
[2021-03-10T07:06:00.934Z]     testFallbackWork(c, tk, cc, "select sum(a) from t")
[2021-03-10T07:06:00.934Z] conn_test.go:817:
[2021-03-10T07:06:00.934Z]     c.Assert(cc.handleQuery(ctx, sql), IsNil)
[2021-03-10T07:06:00.934Z] ... value *errors.withStack = [tikv:9005]Region is unavailable ("[tikv:9005]Region is unavailable")

@lzmhhh123 lzmhhh123 merged commit 610ac87 into pingcap:master Mar 10, 2021
@lzmhhh123 lzmhhh123 deleted the dev/delete_cop_cache_conf branch March 10, 2021 07:37
@mahjonp
Copy link
Contributor

mahjonp commented Mar 18, 2021

We (the benchbot) uses the tikv-client.copr-cache.enable = false to disable copr-cache, and found that thecopr-cache is used by default this week, it's too terrible and inefficient for me to waste time to investigate this incompatible issue.

@mahjonp mahjonp added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Mar 18, 2021
@lzmhhh123
Copy link
Contributor Author

We (the benchbot) uses the tikv-client.copr-cache.enable = false to disable copr-cache, and found that thecopr-cache is used by default this week, it's too terrible and inefficient for me to waste time to investigate this incompatible issue.

copr-cache is used by default before this PR. So not it doesn't break compatibility.

@mahjonp
Copy link
Contributor

mahjonp commented Mar 18, 2021

We (the benchbot) uses the tikv-client.copr-cache.enable = false to disable copr-cache, and found that thecopr-cache is used by default this week, it's too terrible and inefficient for me to waste time to investigate this incompatible issue.

copr-cache is used by default before this PR. So not it doesn't break compatibility.

IMO I think compatibility means it won't break a user's current configuration even it's not the default configuration(you know it's rare for a user that he/she doesn't override a default value)

@ti-srebot ti-srebot mentioned this pull request Mar 19, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackysp jackysp jackysp left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@breezewish breezewish breezewish approved these changes

@zz-jason zz-jason Awaiting requested review from zz-jason

Assignees
No one assigned
Labels
compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. component/config sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lzmhhh123 @bb7133 @XuHuaiyu @ti-chi-bot @mahjonp @breezewish @jackysp @zz-jason