-
Notifications
You must be signed in to change notification settings - Fork 33
Add inbound connections to client pool #630
Conversation
2e52d3c
to
2ba3696
Compare
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.
Looks good.
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.
Just one more test
2ba3696
to
4ae8381
Compare
Does the title still apply? Are we only pooling inbound connections? I'd expect to pull outbound connections too. |
Looking at how small the code change is I am not sure to understand why #630 was split in two. This could have just been parked for until noise is done. |
The issue was added to the sprint backlog so I thought it was ready to be implemented. |
Outbound connections were already being pooled prior to this PR. |
|
||
if let Ok(addr) = peer_addr { | ||
let bam_client = Arc::new(BamClient::new(addr, client)); | ||
debug!("Adding {:?} to list of peers", addr); |
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 log statement should be in the implementation.
4ae8381
to
a859aef
Compare
a859aef
to
821ff5a
Compare
@@ -25,6 +25,7 @@ pub trait Client: Send + Sync + 'static { | |||
|
|||
pub trait ClientFactory<C>: Send + Sync + Debug { | |||
fn client_for(&self, comit_node_socket_addr: SocketAddr) -> Result<Arc<C>, ClientFactoryError>; | |||
fn add_client(&self, comit_node_socket_addr: SocketAddr, client: Arc<C>); |
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.
This should have just been a function on BamClientPool IMO.
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.
Resolves #318