Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

feat: support read throttling by size #938

Closed
wants to merge 3 commits into from

Conversation

Smityz
Copy link
Contributor

@Smityz Smityz commented Oct 19, 2021

@@ -114,6 +114,12 @@ class rpc_holder
return _i->thrift_response;
}

dsn::error_code &error() const
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can add some unit tests for rpc_holder

@hycdong
Copy link
Contributor

hycdong commented Oct 20, 2021

I suggest splitting this pull request into 3 parts:

  1. update with rpc_holder, and add unit test for it.
  2. add token_bucket_throttling_controller
  3. add other logic related with read size throttling

Besides, I notice that you implement token_bucket_throttling_controller under utils folder, but token_bucket_throttling_controller contains logic related with read size, such as function validate, variety _partition_count etc.
If the token_bucket_throttling_controller is a util class, it is supposed NOT contain content about its upper logic, otherwise, it is not suitable under util folder.

@Smityz
Copy link
Contributor Author

Smityz commented Oct 20, 2021

I suggest splitting this pull request into 3 parts:

  1. update with rpc_holder, and add unit test for it.
  2. add token_bucket_throttling_controller
  3. add other logic related with read size throttling

Besides, I notice that you implement token_bucket_throttling_controller under utils folder, but token_bucket_throttling_controller contains logic related with read size, such as function validate, variety _partition_count etc. If the token_bucket_throttling_controller is a util class, it is supposed NOT contain content about its upper logic, otherwise, it is not suitable under util folder.

which folder do you suggest?

@hycdong
Copy link
Contributor

hycdong commented Oct 20, 2021

I suggest splitting this pull request into 3 parts:

  1. update with rpc_holder, and add unit test for it.
  2. add token_bucket_throttling_controller
  3. add other logic related with read size throttling

Besides, I notice that you implement token_bucket_throttling_controller under utils folder, but token_bucket_throttling_controller contains logic related with read size, such as function validate, variety _partition_count etc. If the token_bucket_throttling_controller is a util class, it is supposed NOT contain content about its upper logic, otherwise, it is not suitable under util folder.

which folder do you suggest?

I recommend implement common utility under utils or utility folder, logic related to replica in replication_app_base to let storage module got it.

@Smityz
Copy link
Contributor Author

Smityz commented Oct 20, 2021

The interface refers to https://github.com/XiaoMi/rdsn/blob/master/src/utils/throttling_controller.h
And I also have a plan to use token_bucket_throttling_controller to replace the original one.


if (res.get_value_or(0) > 0) {
if (is_get_perfcounter) {
_reject_task_counter->operator->()->increment();
Copy link
Contributor

Choose a reason for hiding this comment

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

Move _reject_task_counter up to "incubator-pegasus: src/server/pegasus_server_impl.cpp", and increase this counter while return “ERR_BUSY”.

@Smityz Smityz added type/perf-counter PR that made modification on perf-counter, which should be noted in release note. component/throttle Related to rate throttling of Pegasus type/app-envs This PR updates app_envs labels Nov 1, 2021
@Smityz Smityz self-assigned this Nov 1, 2021
@Smityz
Copy link
Contributor Author

Smityz commented Nov 1, 2021

has merged split prs

@Smityz Smityz closed this Nov 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component/throttle Related to rate throttling of Pegasus type/app-envs This PR updates app_envs type/perf-counter PR that made modification on perf-counter, which should be noted in release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants