feat(client): Re-use already connected broker without pending request to fetch metadata

[lbcjbb]

When several brokers are not connected or connected and idle, Client.LeastLoadedBroker() could establish a new connection to a broker rather than favouring an already connected broker. This happends depending on the iteration order of the brokers's map which is not predictable.

This change makes a slight adjustment of this method to return the first connected and idle broker encountered, which may drastically reduce the total number of connection per broker when a same Kafka cluster is used by several thousand applications.

[dnwe]

Thanks, seems like a good refinement - can you add a simple TestLeastLoadedBroker unittest just to lock in the deterministic behaviour - with connected idle broker and a couple of disconnected brokers assert that no connection is opened on the others?

[lbcjbb]

@puellanivis @dnwe Thank you for your review 🙏.
I've just updated the pull request. Please let me know if you spot anything else that needs improving or correcting.

[dnwe]

Hmm, I'm not sure about these most recent updates

Refactoring to add brokerConnectionState and getConnectionState(withTLS bool) just to support this selection logic feels like too much abstraction for a very small feature? The ResponseSize() and Connected() funcs were simple and direct before; now they route through this struct-returning helper that seemingly also optionally inspects TLS state?

The TestClientLeastLoadedBroker is also a lot larger than I was expecting and tightly coupled to implementation details rather than just the observable behaviour

Regarding the change in general, the new priority order seems to be:

1. prefer idle connected broker
2. then prefer any disconnected broker
3. only then use least-loaded connected broker

That means a healthy connected broker with 1 pending request now loses to a completely disconnected broker, forcing a new connection? That doesn't seem to meet the goal that your original issue description was aiming for?

[lbcjbb]

Refactoring to add brokerConnectionState and getConnectionState(withTLS bool) just to support this selection logic feels like too much abstraction for a very small feature? The ResponseSize() and Connected() funcs were simple and direct before; now they route through this struct-returning helper that seemingly also optionally inspects TLS state?

I've added this new func getConnectionState() to avoid to duplicate the logic under ResponseSize() and Connected() inside a new func with a single lock to return these required info, as proposed by @puellanivis. I extended it to TLSConnectionState() just to make all these getter funcs consistent.
If calling ResponseSize() and Connected() is not a problem, I can revert this part.

The TestClientLeastLoadedBroker is also a lot larger than I was expecting and tightly coupled to implementation details rather than just the observable behaviour

I can't see how we could do it any other way, or perhaps by introducing brokers with fake connection when necessary.

That means a healthy connected broker with 1 pending request now loses to a completely disconnected broker, forcing a new connection? That doesn't seem to meet the goal that your original issue description was aiming for?

I’ve been wondering about that. Is it better to prioritise a healthy connected broker, even if this broker is still processing a request? Or assume the costs involve in establishing a new connection to another broker?

[puellanivis]

I was proposing an equally simple and direct third function that returned two pieces of information that would otherwise be acquired through two mutex locks.

I did not suggest that there should be a One Function To Rule Them All™ that each of the simple functions should call into and then return specific values from. We’re allowed to have a little bit of copying… as a treat. Especially, when that little bit of copying keeps everything simple.

Three simple and direct functions heavily outweighs one complicated function called by three functions that then prune out the results they care about.

[puellanivis]

That means a healthy connected broker with 1 pending request now loses to a completely disconnected broker, forcing a new connection? That doesn't seem to meet the goal that your original issue description was aiming for?

Current existing behavior did not account for if the broker was connected or not. So the previous behavior was to prefer the first broker that was either idle, or disconnected. (Both would have a RequestSize of zero.)

We probably do want to switch to preferring the least loaded connected broker, especially if it’s under some magic number of requests. Like you say, a broker with a RequestSize of 1 is probably better than connecting out to another broker. But if all the connected Brokers are all swamped, then we probably really do want to connect out to a disconnected broker.

[lbcjbb]

I was proposing an equally simple and direct third function that returned two pieces of information that would otherwise be acquired through two mutex locks.

I did not suggest that there should be a One Function To Rule Them All™ that each of the simple functions should call into and then return specific values from. We’re allowed to have a little bit of copying… as a treat. Especially, when that little bit of copying keeps everything simple.

Three simple and direct functions heavily outweighs one complicated function called by three functions that then prune out the results they care about.

It's fixed

[lbcjbb]

Do you know how it was implemented in the confluent-kafka-go, or a Kafka library in Python or Java?

[lbcjbb]

In the librdkafka library, rd_kafka_metadata() calls rd_kafka_broker_weight_usable.

 * @brief Weighing function to select a usable broker connections,
 *        promoting connections according to the scoring below.
 *
 * Priority order:
 *  - is not a bootstrap broker
 *  - least idle last 10 minutes (unless blocking)
 *  - least idle hours (if above 10 minutes idle)
 *  - is not a logical broker (these connections have dedicated use and should
 *                             preferably not be used for other purposes)
 *  - is not blocking
 *
 * Will prefer the most recently used broker connection for two reasons:
 * - this connection is most likely to function properly.
 * - allows truly idle connections to be killed by the broker's/LB's
 *   idle connection reaper.
 *
 * Connection must be up.