Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Add inbound connections to client pool #630

Merged
merged 1 commit into from
Jan 14, 2019

Conversation

luckysori
Copy link
Contributor

@luckysori luckysori commented Jan 9, 2019

Resolves #318


  • RTM (Ready To Merge)

@ghost ghost assigned luckysori Jan 9, 2019
@ghost ghost added the review label Jan 9, 2019
@luckysori luckysori force-pushed the 318-reuse-inbound-connections branch from 2e52d3c to 2ba3696 Compare January 9, 2019 22:47
@luckysori luckysori requested review from bonomat, LLFourn and D4nte January 9, 2019 22:47
@D4nte
Copy link
Contributor

D4nte commented Jan 10, 2019

Please update title of #318 accordingly and ensure @LLFourn has done his todo :)

Copy link
Contributor

@D4nte D4nte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

api_tests/dry/rfc003/test.js Show resolved Hide resolved
Copy link
Contributor

@D4nte D4nte left a 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

@luckysori luckysori force-pushed the 318-reuse-inbound-connections branch from 2ba3696 to 4ae8381 Compare January 10, 2019 00:31
@D4nte
Copy link
Contributor

D4nte commented Jan 10, 2019

Does the title still apply? Are we only pooling inbound connections? I'd expect to pull outbound connections too.

@D4nte
Copy link
Contributor

D4nte commented Jan 10, 2019

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.

@luckysori
Copy link
Contributor Author

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.

@luckysori
Copy link
Contributor Author

Does the title still apply? Are we only pooling inbound connections? I'd expect to pull outbound connections too.

Outbound connections were already being pooled prior to this PR.

@luckysori
Copy link
Contributor Author

Please update title of #318 accordingly and ensure @LLFourn has done his todo :)

Now done, epic tracking comit node public keys here.


if let Ok(addr) = peer_addr {
let bam_client = Arc::new(BamClient::new(addr, client));
debug!("Adding {:?} to list of peers", addr);
Copy link
Contributor

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.

@luckysori luckysori force-pushed the 318-reuse-inbound-connections branch from 4ae8381 to a859aef Compare January 14, 2019 06:15
@luckysori luckysori force-pushed the 318-reuse-inbound-connections branch from a859aef to 821ff5a Compare January 14, 2019 06:52
@mergify mergify bot merged commit 142ddfb into master Jan 14, 2019
@ghost ghost removed the review label Jan 14, 2019
@mergify mergify bot deleted the 318-reuse-inbound-connections branch January 14, 2019 07:09
@@ -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>);
Copy link
Collaborator

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.

Copy link
Contributor

@D4nte D4nte Jan 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LLFourn we can correct with #744

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants