CASSGO-50 Endless query execution fix proposal #1875

tengu-alt · 2025-04-08T14:08:17Z

I've researched possible endless query execution when HostSelectionPolicy returns the same downed host (CASSGO-50) and propose three approaches to prevent it.

Wrap hostIter to prevent it from returning downed host before executing the query.
e.g.

  hostIter = func() SelectedHost {
    checkIter := hostIter()
    if !checkIter.Info().IsUp() {
      return nil
    }
    return checkIter
  }

But that approach can cause breaking changes for applications which already relies on user-defined policies that can return downed hosts.

Properly documented type NextHost func():
Add a comment for NextHost type describing that NextHost should return not downedhost or return nil
Add the threshold limit based on the number of hosts inside (q *queryExecutor) do which will prevent the endless execution without the breaking changes

Implementations of the threshold and documentation are presented in this PR.

tengu-alt · 2025-04-08T14:10:45Z

@joao-r-reis @jameshartig can you please take a look at this?

joao-r-reis · 2025-04-21T09:57:49Z

Hmm I don't like the idea of introducing a dependency on the number of connection pools but wrapping hostIter won't solve the issue because whether the host is down or nil doesn't matter much, it will lead to the same behavior...

I actually think it's fine for the driver to assume that the host selection policy is behaving correctly (i.e. not returning the same host over and over) but we still have the issue that in #1793 we added an internal policy that does this (returns the same host everytime the function is called).

I've commented this before but I truly think the best option would be to make the internal host selection policy added in #1793 return the host only once even if that comes at a cost of having no retries or speculative executions when the user uses this feature.

Later down the line if we see that there is demand for retries/speculative executions when the featured added in #1793 is used then we can reevaluate.

One potential alternative if we absolutely want to keep retries when a specific host is being targeted by the query is to add a special flag so that the query executor treats this host selection policy differently (i.e. break from the loop immediately when the returned host is nil or downed).

joao-r-reis · 2025-05-06T15:04:30Z

@tengu-alt do you have time to address this? We're trying to resolve the pending PRs for the 2.0 release

tengu-alt · 2025-05-07T10:36:52Z

@tengu-alt do you have time to address #1875 (comment)? We're trying to resolve the pending PRs for the 2.0 release

Sorry for the late response. I've pushed another approach according to your suggestions.
I propose the deletion of the pool after we got it using the getPoolByHostID, and keep the documentation about policies correct behavior.

connectionpool.go

joao-r-reis · 2025-05-08T14:40:00Z

query_executor.go

-			// we return nil to avoid endless query execution in queryExecutor.do()
-			if !ok || !pool.host.IsUp() {
-				return nil
+			if host != nil {


To ensure this hostIter can be safely called concurrently I think we should make the outer variable an int and then use atomic.CompareAndSwap in this if

returnedHostOnce := 0 hostIter = func() SelectedHost { if atomic.CompareAndSwapInt32(&returnedHostOnce, 0, 1) { return return (*selectedHost)(host) } return nil }

Understood, refactored.

joao-r-reis · 2025-05-08T14:53:13Z

query_executor.go

@@ -89,14 +89,18 @@ func (q *queryExecutor) executeQuery(qry ExecutableQuery) (*Iter, error) {
 	// check if the host id is specified for the query,
 	// if it is, the query should be executed at the corresponding host.
 	if hostID := qry.GetHostID(); hostID != "" {
+		pool, ok := q.pool.getPoolByHostID(hostID)
+		if !ok || !pool.host.IsUp() {


I don't think we need this now that we return nil after the first execution since this check is already being done in the query executor. We needed this before because we needed a way to prevent the hostIter from returning the same unresponsive host over and over again.

Got it, changed.

joao-r-reis · 2025-05-09T13:16:39Z

policies.go

@@ -323,6 +323,7 @@ func (host *selectedHost) Info() *HostInfo {
 func (host *selectedHost) Mark(err error) {}

 // NextHost is an iteration function over picked hosts
+// Should return nil if SelectedHost is not Up to prevent endless query execution.


the driver checks if the host is not up, the function should "eventually" return nil regardless of what causes it.

// Should return nil eventually to prevent endless query execution.

joao-r-reis · 2025-05-09T13:17:30Z

query_executor.go

+		pool, ok := q.pool.getPoolByHostID(hostID)
+		if !ok {
+			return nil, ErrNoConnections
+		}


remove lines 93-96, it's not needed as I explained before

I don't clearly understand: we need the pool to get the host from it.

oh sorry hmm I think it would make more sense to just get the host from the ring here, I didn't even realize it was only getting the pool to get the host before.
Can you replace this with a call to session.ring.getHost(id)? Maybe change getHost to return host, bool so that we can check whether the lookup failed here and return an error

Got it, refactored

joao-r-reis · 2025-05-09T13:18:21Z

query_executor.go

-			if !ok || !pool.host.IsUp() {
-				return nil
+			if atomic.CompareAndSwapInt32(&returnedHostOnce, 0, 1) {
+				return (*selectedHost)(host)


we should add a test to ensure that if a host id is set on the query then it only gets tried once even if it fails

As I checked, TestQuery_SetHostID in the cassandra_test.go already covers this case.

joao-r-reis

👍 nice, can you add to the changelog and format the commit message? Then I'll merge

Fix for the endless query execution when HostSelectionPolicy returns the same downed host. Internal HostSelectionPolicy will return the host only once if HostID is set. Documentation for external policies was added. patch by Oleksandr Luzhniy; reviewed by João Reis, for CASSGO-50

tengu-alt · 2025-05-16T13:13:37Z

👍 nice, can you add to the changelog and format the commit message? Then I'll merge

Done.

tengu-alt force-pushed the fix-endless-execution branch from a11640f to 531ffa0 Compare April 8, 2025 14:09

tengu-alt force-pushed the fix-endless-execution branch from 531ffa0 to 8ea1c0b Compare May 7, 2025 10:32

joao-r-reis reviewed May 8, 2025

View reviewed changes

connectionpool.go Outdated Show resolved Hide resolved

tengu-alt force-pushed the fix-endless-execution branch from 8ea1c0b to 75718a1 Compare May 8, 2025 11:36

joao-r-reis reviewed May 8, 2025

View reviewed changes

tengu-alt force-pushed the fix-endless-execution branch 2 times, most recently from bb18a5c to 570c26e Compare May 9, 2025 10:12

joao-r-reis reviewed May 9, 2025

View reviewed changes

tengu-alt force-pushed the fix-endless-execution branch from 570c26e to 962d4d0 Compare May 15, 2025 11:47

joao-r-reis approved these changes May 16, 2025

View reviewed changes

tengu-alt force-pushed the fix-endless-execution branch from 962d4d0 to c66da4a Compare May 16, 2025 13:13

joao-r-reis merged commit 05e6036 into apache:trunk May 19, 2025
72 checks passed

CASSGO-50 Endless query execution fix proposal #1875

CASSGO-50 Endless query execution fix proposal #1875

Conversation

tengu-alt commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tengu-alt commented Apr 8, 2025

Uh oh!

joao-r-reis commented Apr 21, 2025

Uh oh!

joao-r-reis commented May 6, 2025

Uh oh!

tengu-alt commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

tengu-alt commented May 16, 2025

Uh oh!

Uh oh!

Uh oh!

tengu-alt commented Apr 8, 2025 •

edited

Loading

tengu-alt commented May 7, 2025 •

edited

Loading