CASSGO-121 Improve host_source locking and ring refresh concurrency

[nanassito]

Use RWMutex instead of Mutex to guard fields in host_source

We discovered during a load test of one of our service that there is a lot of locking going on in host_source.go. It turns out that a few can be optimized.

Here are the set of changes in this PR:

1. ringDescriber / GetHosts

The mutex surrounded all of getLocalHostInfo and getClusterPeerInfo (control-connection I/O). prevHosts / prevPartitioner were never written anywhere, so error returns were always nil / "". The mutex only serialized unrelated ring refreshes for no benefit. Removed mu, prevHosts, and prevPartitioner, and return nil, "", err on failure so concurrent GetHosts no longer block each other on network work.

2. HostInfo.ConnectAddressAndPort

It only reads fields; it used Lock and blocked writers. Switched to RLock so it matches other read-only accessors.

This is the most costly one exposed by our load test.

3. HostInfo.HostnameAndPort

Uses a read-optimized path: RLock when hostname is already set (common case); Lock only when lazily filling hostname.

4. isValidPeer

Previously mixed RPCAddress() (under lock) with direct reads of hostId, dataCenter, etc. (racy). One RLock covers all fields and avoids extra lock cycles from a separate RPCAddress call.

5. errorBroadcaster.broadcast

Stopped holding the mutex while sending on listener channels (could stall other newListener / broadcast callers). Snapshot and clear listeners under the lock, then unlock and notify.

This has significantly improved the number of queries per cpu our workload can handle:

Patch by @nanassito ; reviewed by @joao-r-reis and @worryg0d for CASSGO-121

[jon-whit]

We also have this same issue in our organization. Our application is being quite limited in its concurrency burstability because of the lock contention. Here's a sample mutex blocking profile taken from a real production runtime under high load.

[worryg0d]

Hey, I reviewed this patch - jfyi, I'm not an apache committer, so I'm just writing comments.

HostInfo methods might be on a hot path, so it is fine to narrow write-lock to read-lock where it is possible.

Having a benchmark would be useful to see the performance impact.

[worryg0d]

I just realized they are never used

[nanassito]

@worryg0d how set are you on having a comparison benchmark ? it's really out of my way and I'm struggling to make the time to do one. But since the change is trivial I figured it may not be a hard requirement, maybe ?

[worryg0d]

This is not a hard requirement from my side - I assumed you have an application you can bench this change on, apologies if this is not the case.

I'll review it one more time tomorrow to be sure we do not break anything.

However, we will still need one more +1 from commiter. @joao-r-reis could you please take a look at this once you have a chance?

[worryg0d]

LGTM +1

I created CASSGO-121 ticket for this.

@joao-r-reis, please take a look once you have a chance

[nanassito]

friendly ping @joao-r-reis if you can spare the time to review this PR.

[joao-r-reis]

We also have this same issue in our organization. Our application is being quite limited in its concurrency burstability because of the lock contention. Here's a sample mutex blocking profile taken from a real production runtime under high load.

I could be misinterpreting the image but it looks like otelgocql might be responsible for a significant part of the lock contention in your case no? Also if you're using otelgocql that probably means you're using gocql v1 and v1 is no longer supported.

[joao-r-reis]

@nanassito I'm not against making these changes but it feels like we're just doing them for the sake of doing them if we have no benchmark data to verify that these are actually improving the situation. We don't even have data to verify that the issue even exists in the first place...

[joao-r-reis]

We can reopen this once we are provided with some data that helps us troubleshoot this potential issue

[nanassito]

yeah sorry @joao-r-reis we've got this fix in prod so it's quite a bit of work to get the comparison. I don't think it is worth my time to benchmark a comparison between a mutex and a rwmutex for something like this.

[joao-r-reis]

And have you seen a measurable and noticeable impact caused by the changes contained in this PR?

[joao-r-reis]

If you have these changes running in prod and you are able to notice an improvement then that is probably good enough for us to consider merging this in

[nanassito]

Yeah we saw a pretty drastic improvement in queries per cpu we can handle for our workload

[nanassito]

and to be fair, our workload is very silly in calling GetHosts all the time, but this still doesn't have a good reason to fully lock instead of rwlock.

[joao-r-reis]

Can you add this to the CHANGELOG under 2.2.0? And squash commits into a single one (following the commit message guidelines outlined here)

You can put me and @worryg0d as a reviewer on the commit message, I'll ask him if he can take a look at this PR as well

[nanassito]

@joao-r-reis done, I think, please let me know if I did it wrong, this is my first commit on this particular project.

[joao-r-reis]

changelog looks good, the commit message is still missing a line at the end like this:

Patch by nanassito; reviewed by João Reis and Bohdan Siryk for CASSGO-121

[worryg0d]

I have already put my +1 here.

Yeah we saw a pretty drastic improvement in queries per cpu we can handle for our workload

That's a huge improvement. Do you observe it with only this change to the driver? I guess it makes sense if you have a query observer that calls those methods, so narrowing to read lock unlocks queryExecutor.do() when it tries to pick a host to execute the query on.

As Joao said, just add us as reviewers to the commit message, and we're good to merge it

[nanassito]

oh my bad I thought you meant the github PR description instead of the git commit, updating now...

[nanassito]

@joao-r-reis @worryg0d better ?

[joao-r-reis]

Yeah looks good, just letting CI run then will merge, thanks!