-
Notifications
You must be signed in to change notification settings - Fork 26
[MISC] Parallel patroni calls #925
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
Conversation
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
241814d
to
74903f1
Compare
if result := await task: | ||
for task in tasks: | ||
task.cancel() | ||
await wait(tasks) |
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.
Get the first result, cancel the other requests.
) | ||
return cluster_status.json()["members"] | ||
return response["members"] | ||
raise RetryError(last_attempt=Exception("Unable to reach any units")) |
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.
Most existing code should handle RetryErrors instead of empty lists.
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.
Great move!
main() | ||
assert _urlopen.call_args_list == [ | ||
# Iteration 1. server2 is not called | ||
call("http://server1:8008/cluster", timeout=5, context=sentinel.sslcontext), | ||
# Iteration 2 local unit server1 is called first | ||
call("http://server1:8008/cluster", timeout=5, context=sentinel.sslcontext), | ||
call("http://server3:8008/cluster", timeout=5, context=sentinel.sslcontext), | ||
# Iteration 3 Last known member is server3 | ||
call("https://server3:8008/cluster", timeout=5, context=sentinel.sslcontext), | ||
] |
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.
Order is no longer deterministic.
) | ||
cluster_status = json.loads(resp.read()) | ||
loop = get_running_loop() | ||
tasks = [loop.run_in_executor(None, call_url, url, context) for url in urls] |
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.
No deps when executing the script, so running urllib requests in an executor.
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 have run tests, the deployment of 3 units cluster is about 5-10% faster (5 minutes instead of 5-6). The defers count is also slightly better (110 VS 127 for 3 units deployment).
Also topology observer behaves properly on Primary loose (without leader re-elected).
LGTM. Good work and the step into the proper direction for sure!
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.
Great work on introducing async in the proper way to improve the performance of those calls, Dragomir!
LGTM!
) | ||
return cluster_status.json()["members"] | ||
return response["members"] | ||
raise RetryError(last_attempt=Exception("Unable to reach any units")) |
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.
Great move!
Async parallel requests for cluster status.
Checklist