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

*: support grpc memory quota #5818

Merged
merged 7 commits into from Nov 8, 2019
Merged

Conversation

@hunterlxt
Copy link
Contributor

hunterlxt commented Nov 5, 2019

Signed-off-by: TXXT hunterlxt@live.com

What have you changed?

  • support memory quota

Enable memory size quota feature added in grpc-rs 0.5.0-alpha.5 to control memory usage to avoid OOM.

What is the type of the changes?

  • New feature (a change which adds functionality)

How is the PR tested?

  • Integration test

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

YES

Does this PR affect tidb-ansible?

YES

hunterlxt added 2 commits Nov 5, 2019
Signed-off-by: TXXT <hunterlxt@live.com>
@hunterlxt hunterlxt self-assigned this Nov 5, 2019
@hunterlxt hunterlxt added the C: gRPC label Nov 5, 2019
@hunterlxt hunterlxt requested review from BusyJay and nrc and removed request for BusyJay Nov 5, 2019
## Limit the memory size can be used by gRPC. Default is unlimited.
## gRPC usually works well to reclaim memory by itself. Limit the memory in case OOM
## is observed. Note that limit the usage can lead to potential stall.
# grpc-memory-pool-quota = "32G"

This comment has been minimized.

Copy link
@siddontang

siddontang Nov 6, 2019

Contributor

em, is default 32G too large?

This comment has been minimized.

Copy link
@hunterlxt

hunterlxt Nov 6, 2019

Author Contributor

32G just indicates "big quota size". Because exceeding the limit may cause some unpredictable stall (meaning that the stream is disconnected, the channel refuses to connect, etc.). By default, I don't want this to cause potential problems.

Copy link
Contributor

nrc left a comment

lgtm

@@ -19,6 +19,7 @@ const DEFAULT_STATUS_ADDR: &str = "127.0.0.1:20180";
const DEFAULT_GRPC_CONCURRENCY: usize = 4;
const DEFAULT_GRPC_CONCURRENT_STREAM: i32 = 1024;
const DEFAULT_GRPC_RAFT_CONN_NUM: usize = 1;
const DEFAULT_GRPC_MEMORY_POOL_QUOTA: ReadableSize = ReadableSize(isize::MAX as u64);

This comment has been minimized.

Copy link
@nrc

nrc Nov 6, 2019

Contributor

I think this should be a u64, following DEFAULT_GRPC_STREAM_INITIAL_WINDOW_SIZE

This comment has been minimized.

Copy link
@hunterlxt

hunterlxt Nov 6, 2019

Author Contributor

Agree

Signed-off-by: TXXT <hunterlxt@live.com>
@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 6, 2019

/run-all-tests

@BusyJay
BusyJay approved these changes Nov 6, 2019
@BusyJay BusyJay added the S: LGT1 label Nov 6, 2019
@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 6, 2019

/run-unit-test

@hunterlxt hunterlxt requested a review from siddontang Nov 6, 2019
@nrc
nrc approved these changes Nov 6, 2019
Copy link
Contributor

nrc left a comment

Looks good, thanks for the changes

nrc and others added 2 commits Nov 6, 2019
@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 7, 2019

/merge

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

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

Your auto merge job has been accepted, waiting for 5736, 5779, 5473

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 7, 2019

@hunterlxt merge failed.

@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 7, 2019

/run-integration-copr-test

@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 8, 2019

/merge

@hunterlxt

This comment has been minimized.

Copy link
Contributor Author

hunterlxt commented Nov 8, 2019

/run-all-tests

1 similar comment
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 8, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 8, 2019

@hunterlxt merge failed.

@hunterlxt hunterlxt merged commit afdc034 into tikv:master Nov 8, 2019
5 of 6 checks passed
5 of 6 checks passed
idc-jenkins-ci-tikv/integration-copr-test Jenkins job failed
Details
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
@hunterlxt hunterlxt deleted the hunterlxt:lxt/mem-quota-master branch Nov 8, 2019
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Signed-off-by: TXXT <hunterlxt@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.