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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gc: config gc io limit dynamically #5769

Merged
merged 9 commits into from Nov 18, 2019
Merged

Conversation

@youjiali1995
Copy link
Contributor

youjiali1995 commented Oct 31, 2019

Signed-off-by: youjiali1995 zlwgx1023@gmail.com

What have you changed?

tikv-ctl can change gc io limit dynamically now. Usage:

tikv-ctl --host=host modify-tikv-config -m server -n gc.max_write_bytes_per_sec -v 10MB

It's used to limit the write flow when gc influences normal requests, and remove the limit when there is no influence.

What is the type of the changes?

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

How is the PR tested?

  • Unit test
  • Manual test (add detailed scripts or steps below)
    image

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Yes. I will file one.

Does this PR affect tidb-ansible?

No

Refer to a related PR or issue link (optional)

#5263

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Oct 31, 2019

The implementation may be changed totally because many configurations will be modified dynamically this quarter. So only gc io limiter is supported in this PR.

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Oct 31, 2019

/run-integration-common-test

@overvenus

This comment has been minimized.

Copy link
Contributor

overvenus commented Nov 4, 2019

FYI @NingLin-P , this PR changes GC config dynamically.

src/server/gc_worker.rs Outdated Show resolved Hide resolved
@sticnarf

This comment has been minimized.

Copy link
Contributor

sticnarf commented Nov 8, 2019

Is it possible that accumulated GC tasks blocks GCTask::ChangeIoLimit from executing?

For example, originally we set the value too small and find the speed too slow. Then, we want to use tikv-ctl to make the limit larger. However, the previous GC task runs too slow so that the change limit task cannot run. Can we solve the problem?

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 11, 2019

Is it possible that accumulated GC tasks blocks GCTask::ChangeIoLimit from executing?

For example, originally we set the value too small and find the speed too slow. Then, we want to use tikv-ctl to make the limit larger. However, the previous GC task runs too slow so that the change limit task cannot run. Can we solve the problem?

Yes, it's possible. I can create the IoLimiter protected by a mutex in the GCWorker and pass it to the GCRunner. Then we can change the limit in the GCWorker.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 12, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

Copy link
Contributor

sticnarf left a comment

I suggest using RwLock because the limiter is accessed frequently but seldom changes. (I know the time acquring lock is really small compared to the whole gc operation. It's more reasonable :)

The rest LGTM

Copy link
Contributor

MyonKeminta left a comment

LGTM

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

I suggest using RwLock because the limiter is accessed frequently but seldom changes. (I know the time acquring lock is really small compared to the whole gc operation. It's more reasonable :)

The rest LGTM

@sticnarf I think changing limit and requesting quota are both write operations.

@sticnarf

This comment has been minimized.

Copy link
Contributor

sticnarf commented Nov 18, 2019

But IOLimiter::request only needs &self...

Copy link
Contributor

sticnarf left a comment

Anyway. It's not so important.

LGTM

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Nov 18, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

/run-all-tests

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

@youjiali1995 merge failed.

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

4 similar comments
@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Nov 18, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

Your auto merge job has been accepted, waiting for 5903

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

/run-all-tests

@sre-bot sre-bot merged commit 1d5e120 into tikv:master Nov 18, 2019
6 checks passed
6 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-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

cherry pick to release-3.0 failed

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 18, 2019

cherry pick to release-3.1 failed

b41sh added a commit to b41sh/tikv that referenced this pull request Nov 18, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Nov 19, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Nov 19, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@youjiali1995 youjiali1995 deleted the youjiali1995:dynamically-config-gc branch Nov 19, 2019
sre-bot added a commit that referenced this pull request Nov 20, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Nov 25, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
sre-bot added a commit that referenced this pull request Nov 25, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.