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

INDY-1602: Dynamic quota control #905

Merged
merged 8 commits into from
Aug 31, 2018

Conversation

skhoroshavin
Copy link
Member

No description provided.

Sergey Khoroshavin added 4 commits August 29, 2018 19:03
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
…nto node

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
max_node_quota: Quota,
max_client_quota: Quota):
self._dynamic = dynamic
self._max_request_queue_size = max_request_queue_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the prefix max here? quota already means that this is some max limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is max quota which this strategy will ever allow, so I believe max is appropriate here.

def client_quota(self) -> Quota:
if not self._dynamic:
return self._max_client_quota
return Quota(count=0, size=0) if self._request_queue_overflow else self._max_client_quota
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like one of possible strategies. Should we make QuotaControl an abstract class, and have a strategy QuotaControlNoClientMsgsOnRequestsOverflow?

# Zeromq configuration
# Quotas configuration
ENABLE_DYNAMIC_QUOTAS = False
MAX_REQUEST_QUEUE_SIZE = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it too much? If we have a load of 10 writes per sec, than the client stack will be stopped if we don't order anything for 1000 secs.

@@ -1098,6 +1107,8 @@ def reset(self):
self.metrics.add_event(MetricsName.LOOPER_RUN_TIME_SPENT, time.perf_counter() - self.last_prod_started)
self.last_prod_started = time.perf_counter()

self.quota_control.set_request_queue_len(len(self.requests))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a possible issue here:
As of now we have a problem (INDY-1248) that a request is not removed from the requests queue if at least one instance hasn't yet ordered it. If one of the instances doesn't have a Primary, then the requests queue will grow even if everything is totally fine on master and other instances.

Sergey Khoroshavin added 3 commits August 31, 2018 12:28
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@skhoroshavin
Copy link
Member Author

(ci) test this please

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
@@ -64,6 +64,7 @@ def is_ordered_by_all(self):
def __init__(self, instance_count):
self.instance_count = instance_count
self._requests = {}
self._unordered = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we already have such a field? How do we track requests unordered for some time?

Copy link
Member Author

Choose a reason for hiding this comment

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

We track them using _requests dictionary. The problem is that it contains requests unordered by any instance, and this list can become quite big if we have some slow backup instance. So I added separate set of requests which are unordered just by master.

@ashcherbakov ashcherbakov merged commit b6cff77 into hyperledger:master Aug 31, 2018
@skhoroshavin skhoroshavin deleted the indy-1602 branch August 31, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants