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 memory quota #5524
*: support memory quota #5524
Conversation
- support memory quota - fix grpc coredump issue Signed-off-by: Jay Lee <busyjaylee@gmail.com>
|
||
- Limit the memory size can be used by gRPC | ||
- Default: 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is potential stall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocation stall. It will not allocate until there are free slots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no free memory, the connection(Channel) is refused directly. Maybe it should be explained.
## 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc-memory-pool-quota
represents the memory usage for each connect. So 32G will it be too large?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when it's attached to the server, it should be a limit to the memory server uses, not just a single connection. Besides, 32G is just a hint for unlimited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine
@@ -63,6 +63,7 @@ pub struct Config { | |||
pub grpc_compression_type: GrpcCompressionType, | |||
pub grpc_concurrency: usize, | |||
pub grpc_concurrent_stream: i32, | |||
pub grpc_memory_pool_quota: ReadableSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use usize
just like grpc_concurrency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it has unit. Values like "32GiB" or "1024MiB" should be valid configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
What have you changed?
What is the type of the changes?
How is the PR tested?
integration tests
Does this PR affect documentation (docs) or should it be mentioned in the release notes?
Yes.
Does this PR affect
tidb-ansible
?Yes.