connect_and_check: Allow choosing which connections to refresh. #112
Conversation
nihohit
force-pushed
the
connect-and-check
branch
2 times, most recently
from
7968500 to
486af81
Compare
| addr, | ||
| params.clone(), | ||
| None, | ||
| RefreshConnectionType::OnlyUserConnection, |
There was a problem hiding this comment.
If the DNS has changed, why are we refreshing only the user connection?
There was a problem hiding this comment.
This PR tries to minimize changes to get_or_create_conn, and ATM we don't use management connections.
| where | ||
| C: ConnectionLike + Connect + Send + Clone + 'static, | ||
| C: ConnectionLike + Send + Clone + Sync + Connect + 'static, | ||
| { | ||
| if let Some(node) = node { | ||
| let mut conn = node.user_connection.await; |
There was a problem hiding this comment.
this line can be moved under the dns check
| Err(_) => connect_and_check(addr, params.clone(), None).await, | ||
| Ok(_) => Ok(AsyncClusterNode::new( | ||
| async { conn }.boxed().shared(), | ||
| None, |
There was a problem hiding this comment.
so in this PR you're not adding the management connection yet, just adding the functionality?
There was a problem hiding this comment.
This PR is large enough, as it is. I don't want to make it larger than it needs to be.
| #[doc(hidden)] | ||
| #[must_use] | ||
| pub enum ConnectAndCheckResult<C> { | ||
| Success(AsyncClusterNode<C>), |
There was a problem hiding this comment.
maybe add a comment explaining each one, something like: Success -> All required connections succeeded, ManagementConnectionFailed => Only the management connection failed to be established, Failed => all required connections failed
| }; | ||
|
|
||
| if let Some(new_conn) = management_connection.as_mut() { | ||
| // The new IP matches the existing one. Use this connection for the management connection. |
There was a problem hiding this comment.
This comment should be moved to the else in line 262
| } | ||
| } | ||
|
|
||
| async fn create_management_connection<C>( |
There was a problem hiding this comment.
I think this function is redundant, we can add the warning into the ManagementConnectionFailed result constructor
| } | ||
| } | ||
|
|
||
| async fn create_user_connection<C>( |
There was a problem hiding this comment.
create_and_setup_management_connection and create_user_connection names should be the same in all but the connection name (user/management)
| ) -> (AsyncClusterNode<MockConnection>, redis::RedisError) { | ||
| match result { | ||
| ConnectAndCheckResult::ManagementConnectionFailed { node, err } => (node, err), | ||
| ConnectAndCheckResult::Success(_) => panic!("full success"), |
There was a problem hiding this comment.
please make the panic messages more informative
| .await; | ||
| let (node, _) = assert_partial_result(result); | ||
| assert!(node.management_connection.is_none()); | ||
| assert_eq!(node.ip, None); |
| .await; | ||
| let (node, _) = assert_partial_result(result); | ||
| assert!(node.management_connection.is_none()); | ||
| assert_eq!(node.ip, None); |
|
@barshaul round |
nihohit
force-pushed
the
connect-and-check
branch
from
b752e78 to
6e2739c
Compare
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.