Wrap cluster connection management in object. #42
Conversation
nihohit
force-pushed
the
connection-container
branch
from
a751d7f
to
a5da6d7
Compare
| connections.insert(addr, async { conn }.boxed().shared()); | ||
| &mut *connections_container, | ||
| |connections_container, identifier| async move { | ||
| let addr_option = connections_container.address_for_identifier(&identifier); |
There was a problem hiding this comment.
address_for_identifier should always return the address, regardless if it's found in the current connection map
There was a problem hiding this comment.
you got a whole commit, just for one comment!
| Some(conn.clone()) | ||
| } | ||
|
|
||
| pub(crate) fn connection_for_address( |
There was a problem hiding this comment.
How would this function work if the identifier would be something different from the address?
There was a problem hiding this comment.
I mean - or we'll need another map to save the addresses to their identifiers (then we also need to have a hash lookup which this solution should save), or we'll have the address already saved with the identifier - then we don't need to use the address at all. Or did you think about something else?
There was a problem hiding this comment.
it depends on what the identifier is. for example, the identifier might be usize, and we'll have something like
struct Identifier(usize);
struct connections_manager {
connections: Vec<Option<C>>,
slot_map: BTree<usize, usize>, // the typing here is more complex, but the gist is - slot to index
addresses_map: HahsMap<String, usize>
}then each identifier just goes into self.connections[*identifier], and saves us a hash lookup on the most common scenario - slot to connection, and on somewhat common scenario - retry to same node.
| identifier | ||
| } | ||
|
|
||
| pub(crate) fn remove_connection(&mut self, identifier: &Identifier) -> Option<Connection> { |
There was a problem hiding this comment.
so, if we remove the connection only from the connection map, but the node's address is still present in the slot map, and we have a command that routes to a slot that points to this node address.
So we'll try to get the connection from the address and find that we don't have it in. So we'll try to create a new connection to this node, and if it fails we will send the command to a random node, even though we already decided to remove this connection in the refresh_connections function. not for this PR but we need to understand how to handle changes only in the connection map and when and how it should trigger a slots refreshment
There was a problem hiding this comment.
once you have an identifier, a missing connection always means that a disconnect happened. That's one of the benefits of using strong typing - identifiers are only creatable by the connection_manager.
There was a problem hiding this comment.
currently it isn't true - we're searching for a connection based on its address, so a missing connection might say a disconnect happened or it's a new node. Am I wrong?
There was a problem hiding this comment.
no, the API says that you search for a connection based on an identifier. The user doesn't know that an identifier contains the address, and in future refactors, identifiers might not contain these addresses.
Of course, there's another problematic scenario - what happens if you have an existing identifier from an old container, but the connections container was replaced? The new container might not contain the address.
|
@barshaul round |
Added connections_manager, a single object that handles addition, removal, and fetching of connections. This will allow us to easily modify behavior, for example, by adding round-robin read from replica.
nihohit
force-pushed
the
connection-container
branch
from
b8e9891
to
4adbea9
Compare
Added connections_manager, a single object that handles addition, removal, and fetching of connections. This will allow us to easily modify behavior, for example, by adding round-robin read from replica.