refactor: Add a little testability, abstract a bit over broker kinds #74
Conversation
There was a problem hiding this comment.
The interface / abstraction point is good, I'm a bit confused about naming things (which IIRC is the 2nd hard problem of computer science).
This conflicts a bit w/ #72 but it shouldn't be too hard of either of us to rebase (depending on who goes first) since it's effectively only affecting the arguments that Connct::connection is going to pass through to Transport.
src/connection.rs
Outdated
| struct FakeBrokerConn { | ||
| conn: Box<dyn Fn() -> Result<Arc<FakeConn>> + Send + Sync>, | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq)] | ||
| struct FakeConn; |
There was a problem hiding this comment.
The names confuse me a bit. What's the difference between a "fake broker conn(ection)" and a "fake conn(ection)"?
There was a problem hiding this comment.
Yeaaahhh I didn't put a whole lot of thought into the names 😅 I renamed FakeBrokerConn to FakeBrokerRepresentation, is that better?
| @@ -37,14 +37,79 @@ pub enum Error { | |||
|
|
|||
| pub type Result<T, E = Error> = std::result::Result<T, E>; | |||
|
|
|||
| /// How to connect to a `Transport` | |||
There was a problem hiding this comment.
If you connect to a "transport", then why is async fn connection returning R: Request? Is this a bit of a naming clash because Transport was already taken by crate::connection::transport::Transport? Maybe we can call Request a RequestHandler. I think nouns are better for traits and structs and verbs rather for functions. Same goes for Connect::connection which I would rather call ConnectionHandler::connect, esp. since the method actually establishes a new connection rather than using a cached one (we currently don't cache connections at this specific layer and I think we don't want to because Kafka doesn't really multiplex TCP connection requests but only allows in-order pipelining).
There was a problem hiding this comment.
Yup, these names were not great. I've taken your suggestions here, how does it look now?
carols10cents
force-pushed
the
cn/explore
branch
2 times, most recently
from
a88db1f to
71a6007
Compare
… method on BrokerConnector
…ither bootstrap or topology brokers
Co-authored-by: Marco Neumann <[email protected]>
carols10cents
force-pushed
the
cn/explore
branch
from
71a6007 to
32315f8
Compare
|
Because you approved, I'm going to merge this. If we need to adjust names more, I'm happy to do so in a future PR! |
Connects to #49, in that it adds a tiny bit of testing around getting a new arbitrary broker connection to cache.
This also adds a bit of type info around different sources of broker connection, and fixes a subtle bug where broker id was being logged as
Noneeven though we had a broker ID from the topology.Again recommended to review commit-by-commit.
I feel less confident about the value of these extractions and the new test. If the new test fails, it's likely to be a flaky failure too because of the randomness.
Looking at other code like the controller and partition leader caching, I think perhaps the broker caching trait could be reused, and then perhaps the
maybe_retryfunctions could be shared more and tested more? Not sure yet though.