-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Rewrite net::client::redundant
#462
base: main
Are you sure you want to change the base?
Conversation
The new implementation is significantly simpler and shorter. There are two important changes to discuss: - The 'Transport' type and worker task have been replaced by a simple concurrent data structure within 'Connection'. This eliminates an unnecessary synchronization point for performing requests: now, many requests can be launched from multiple threads simultaneously. - The 'QueryState' based state machine has been replaced with a simple async function definition, that iterates through transports and then falls back to waiting on all running connections. It is far easier to understand the control flow here. - Instead of 'tokio::select!{}' in a loop, we use 'buffer_unordered' to dynamically collect ongoing requests (which we produce following timeouts). - Instead of tracking IDs for transports, we wrap them in 'Arc's and then give out references to them. Transport statistics can then be updated directly, without external communication. - Instead of 'QueryState::Report()', we use a drop guard in the underlying request futures. This directly writes to a statistics structure that is shared with all threads. The end result is a smaller, more readable codebase. 'Connection::new' no longer returns a separate 'Transport' type. 'Connection::add' has become a synchronous, infallible function. Multiple requests can be performed in parallel, without having to request data from a task.
This removes a layer of vtables that may not be necessary in some applications. However, it complicates the typing -- we'll see whether it's worthwhile.
tokio::time::sleep(timeout) | ||
.map(|()| Some(transport.transport.request(request))) | ||
}) | ||
// Execute all requests concurrently, as they are produced. |
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 this is wrong. A next request starts only after the previous one had enough time to complete.
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.
Each transport request is only "produced" after the respective timeout is executed. The overall flow looks like:
transport 1 | timeout | transport 2 | timeout | ...
\------------------------------------------------- request 1
\------------------------- request 2
\- ...
Should I add a comment at the top of this function to make it more explicit?
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've added some comments; let me know if they make the code easier to understand.
}); | ||
let redun = redundant::Connection::< | ||
Box<dyn SendRequest<RequestMessage<Vec<u8>>> + Send + Sync>, | ||
>::new(); |
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 don't like how the interface got worse in this respect. Having spell out Box<dyn SendRequest<...>> is quite unfriendly compared to what it was.
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.
Yeah, that's fair. I was trying out the fully strongly typed interface, but I'm happy to revert it to always using Box<dyn SendRequest>
, or to just set Box<dyn SendRequest>
as the default for that generic parameter.
This is a low-priority PR for simplifying the
Redundant
client transport. I've managed to replace a large and difficult state machine with a much simpler control flow, making good use of thefutures_util
crate. The module has become significantly smaller and easier to understand.