Skip to content

Commit 7da1f87

Browse files
committed
3.x: fail fast on keyspace setup validation failures
Fixes #940. Part of #939. Jira: https://scylladb.atlassian.net/browse/DRIVER-751 Keyspace setup during connection borrow runs an internal USE before the user query is written. Treat validation failures from that USE as permanent request errors instead of connection or host failures: preserve QueryValidationException, avoid defuncting the connection, and stop the request without trying the next host or invoking retry policy. Keep driver-side and transient server-side keyspace setup failures on the existing pre-query next-host path. Report synchronous setKeyspaceAsync write failures through failed futures, clear failed attempts, and restore pool accounting for borrow and dequeue failures. Add regression coverage for 0x2200 INVALID responses, synchronous setKeyspace validation handling, client/server-side keyspace setup timeouts, pool accounting cleanup, retry-policy accounting, and next-host behavior.
1 parent 150f4e7 commit 7da1f87

9 files changed

Lines changed: 879 additions & 265 deletions

File tree

driver-core/src/main/java/com/datastax/driver/core/Connection.java

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import com.datastax.driver.core.exceptions.DriverInternalError;
3535
import com.datastax.driver.core.exceptions.FrameTooLongException;
3636
import com.datastax.driver.core.exceptions.OperationTimedOutException;
37+
import com.datastax.driver.core.exceptions.QueryValidationException;
3738
import com.datastax.driver.core.exceptions.TransportException;
3839
import com.datastax.driver.core.exceptions.UnsupportedProtocolVersionException;
3940
import com.datastax.driver.core.utils.MoreFutures;
@@ -845,17 +846,15 @@ void setKeyspace(String keyspace) throws ConnectionException {
845846

846847
try {
847848
Uninterruptibles.getUninterruptibly(setKeyspaceAsync(keyspace));
848-
} catch (ConnectionException e) {
849-
throw defunct(e);
850-
} catch (BusyConnectionException e) {
851-
logger.warn(
852-
"Tried to set the keyspace on busy {}. "
853-
+ "This should not happen but is not critical (it will be retried)",
854-
this);
855-
throw new ConnectionException(endPoint, "Tried to set the keyspace on busy connection");
856849
} catch (ExecutionException e) {
857850
Throwable cause = e.getCause();
858-
if (cause instanceof OperationTimedOutException) {
851+
if (cause instanceof BusyConnectionException) {
852+
throw keyspaceBusyException();
853+
} else if (cause instanceof QueryValidationException) {
854+
throw (QueryValidationException) cause;
855+
} else if (cause instanceof ConnectionException) {
856+
throw defunct((ConnectionException) cause);
857+
} else if (cause instanceof OperationTimedOutException) {
859858
// Rethrow so that the caller doesn't try to use the connection, but do not defunct as we
860859
// don't want to mark down
861860
logger.warn(
@@ -869,8 +868,15 @@ void setKeyspace(String keyspace) throws ConnectionException {
869868
}
870869
}
871870

872-
ListenableFuture<Connection> setKeyspaceAsync(final String keyspace)
873-
throws ConnectionException, BusyConnectionException {
871+
private ConnectionException keyspaceBusyException() {
872+
logger.warn(
873+
"Tried to set the keyspace on busy {}. "
874+
+ "This should not happen but is not critical (it will be retried)",
875+
this);
876+
return new ConnectionException(endPoint, "Tried to set the keyspace on busy connection");
877+
}
878+
879+
ListenableFuture<Connection> setKeyspaceAsync(final String keyspace) {
874880
SetKeyspaceAttempt existingAttempt = targetKeyspace.get();
875881
if (MoreObjects.equal(existingAttempt.keyspace, keyspace)) return existingAttempt.future;
876882

@@ -900,7 +906,18 @@ ListenableFuture<Connection> setKeyspaceAsync(final String keyspace)
900906
logger.debug("{} Setting keyspace {}", this, keyspace);
901907
// Note: we quote the keyspace below, because the name is the one coming from Cassandra, so
902908
// it's in the right case already
903-
Future future = write(new Requests.Query("USE \"" + keyspace + '"'));
909+
Future future;
910+
try {
911+
future = write(new Requests.Query("USE \"" + keyspace + '"'));
912+
} catch (ConnectionException | BusyConnectionException e) {
913+
targetKeyspace.compareAndSet(attempt, defaultKeyspaceAttempt);
914+
ksFuture.setException(e);
915+
return ksFuture;
916+
} catch (RuntimeException e) {
917+
targetKeyspace.compareAndSet(attempt, defaultKeyspaceAttempt);
918+
ksFuture.setException(e);
919+
return ksFuture;
920+
}
904921
Futures.addCallback(
905922
future,
906923
new FutureCallback<Message.Response>() {
@@ -915,7 +932,12 @@ public void onSuccess(Message.Response response) {
915932
targetKeyspace.compareAndSet(attempt, defaultKeyspaceAttempt);
916933
if (response.type == ERROR) {
917934
Responses.Error error = (Responses.Error) response;
918-
ksFuture.setException(defunct(error.asException(endPoint)));
935+
DriverException exception = error.asException(endPoint);
936+
if (exception instanceof QueryValidationException) {
937+
ksFuture.setException(exception);
938+
} else {
939+
ksFuture.setException(defunct(exception));
940+
}
919941
} else {
920942
ksFuture.setException(
921943
defunct(

driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import static com.datastax.driver.core.Connection.State.TRASHED;
2828

2929
import com.datastax.driver.core.exceptions.AuthenticationException;
30+
import com.datastax.driver.core.exceptions.BusyConnectionException;
3031
import com.datastax.driver.core.exceptions.BusyPoolException;
3132
import com.datastax.driver.core.exceptions.ConnectionException;
3233
import com.datastax.driver.core.exceptions.UnsupportedProtocolVersionException;
@@ -611,7 +612,22 @@ ListenableFuture<Connection> borrowConnection(
611612
if (totalInFlightCount > currentCapacity) maybeSpawnNewConnection(shardId);
612613
}
613614

614-
return leastBusy.setKeyspaceAsync(manager.poolsState.keyspace);
615+
final Connection borrowedConnection = leastBusy;
616+
ListenableFuture<Connection> setKeyspaceFuture =
617+
borrowedConnection.setKeyspaceAsync(manager.poolsState.keyspace);
618+
Futures.addCallback(
619+
setKeyspaceFuture,
620+
new FutureCallback<Connection>() {
621+
@Override
622+
public void onSuccess(Connection connection) {}
623+
624+
@Override
625+
public void onFailure(Throwable t) {
626+
borrowedConnection.release(t instanceof BusyConnectionException);
627+
}
628+
},
629+
MoreExecutors.directExecutor());
630+
return setKeyspaceFuture;
615631
}
616632

617633
private ListenableFuture<Connection> enqueue(

driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,13 @@
2424
import com.codahale.metrics.Timer;
2525
import com.datastax.driver.core.exceptions.BootstrappingException;
2626
import com.datastax.driver.core.exceptions.BusyConnectionException;
27-
import com.datastax.driver.core.exceptions.BusyPoolException;
2827
import com.datastax.driver.core.exceptions.ConnectionException;
2928
import com.datastax.driver.core.exceptions.DriverException;
3029
import com.datastax.driver.core.exceptions.DriverInternalError;
3130
import com.datastax.driver.core.exceptions.NoHostAvailableException;
3231
import com.datastax.driver.core.exceptions.OperationTimedOutException;
3332
import com.datastax.driver.core.exceptions.OverloadedException;
33+
import com.datastax.driver.core.exceptions.QueryValidationException;
3434
import com.datastax.driver.core.exceptions.ReadFailureException;
3535
import com.datastax.driver.core.exceptions.ReadTimeoutException;
3636
import com.datastax.driver.core.exceptions.ServerError;
@@ -468,15 +468,11 @@ public void onSuccess(Connection connection) {
468468

469469
@Override
470470
public void onFailure(Throwable t) {
471-
if (t instanceof BusyPoolException) {
472-
logError(host.getEndPoint(), t);
473-
} else {
474-
logger.warn(
475-
"Unexpected error while querying {} - [{}]. Find next host to query.",
476-
host.getEndPoint(),
477-
t.toString());
478-
logError(host.getEndPoint(), t);
471+
if (t instanceof QueryValidationException) {
472+
setFinalException(null, (QueryValidationException) t);
473+
return;
479474
}
475+
logError(host.getEndPoint(), t);
480476
findNextHostAndQuery();
481477
}
482478
},

driver-core/src/main/java/com/datastax/driver/core/policies/DefaultRetryPolicy.java

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@
3434
* <li>On a write timeout, retries once on the same host if we timeout while writing the
3535
* distributed log used by batch statements.
3636
* <li>On an unavailable exception, retries once on the next host.
37-
* <li>On a request error, such as a client timeout, retries once on the next host. Do not retry
38-
* on read or write failures.
37+
* <li>On a request error, such as a client timeout, the query is retried on the next host. Do not
38+
* retry on read or write failures.
3939
* </ul>
4040
*
4141
* <p>This retry policy is conservative in that it will never retry with a different consistency
@@ -136,27 +136,16 @@ public RetryDecision onUnavailable(
136136
return (nbRetry == 0) ? RetryDecision.tryNextHost(null) : RetryDecision.rethrow();
137137
}
138138

139-
/**
140-
* {@inheritDoc}
141-
*
142-
* <p>This implementation triggers a maximum of one retry on the next host in the query plan. The
143-
* rationale is that the first coordinator might have been network-isolated or overloaded, and
144-
* moving to the next host might resolve the issue. If the retry also fails, the exception is
145-
* rethrown.
146-
*
147-
* <p>Read and write failures are never retried, as they generally indicate a data problem that is
148-
* unlikely to be resolved by a retry.
149-
*
150-
* @return {@code RetryDecision.tryNextHost(cl)} if no retry attempt has yet been tried and the
151-
* error is not a read/write failure, {@code RetryDecision.rethrow()} otherwise.
152-
*/
139+
/** {@inheritDoc} */
153140
@Override
154141
public RetryDecision onRequestError(
155142
Statement statement, ConsistencyLevel cl, DriverException e, int nbRetry) {
143+
// do not retry these by default as they generally indicate a data problem or
144+
// other issue that is unlikely to be resolved by a retry.
156145
if (e instanceof WriteFailureException || e instanceof ReadFailureException) {
157146
return RetryDecision.rethrow();
158147
}
159-
return (nbRetry == 0) ? RetryDecision.tryNextHost(cl) : RetryDecision.rethrow();
148+
return RetryDecision.tryNextHost(cl);
160149
}
161150

162151
@Override

driver-core/src/main/java/com/datastax/driver/core/policies/DowngradingConsistencyRetryPolicy.java

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -208,24 +208,21 @@ public RetryDecision onUnavailable(
208208
/**
209209
* {@inheritDoc}
210210
*
211-
* <p>This implementation triggers a maximum of one retry on the next host in the query plan. The
212-
* rationale is that the first coordinator might have been network-isolated or overloaded, and
213-
* moving to the next host might resolve the issue. If the retry also fails, the exception is
214-
* rethrown.
215-
*
216-
* <p>Read and write failures are never retried, as they generally indicate a data problem that is
217-
* unlikely to be resolved by a retry.
218-
*
219-
* @return {@code RetryDecision.tryNextHost(cl)} if no retry attempt has yet been tried and the
220-
* error is not a read/write failure, {@code RetryDecision.rethrow()} otherwise.
211+
* <p>For historical reasons, this implementation triggers a retry on the next host in the query
212+
* plan with the same consistency level, regardless of the statement's idempotence. Note that this
213+
* breaks the general rule stated in {@link RetryPolicy#onRequestError(Statement,
214+
* ConsistencyLevel, DriverException, int)}: "a retry should only be attempted if the request is
215+
* known to be idempotent".`
221216
*/
222217
@Override
223218
public RetryDecision onRequestError(
224219
Statement statement, ConsistencyLevel cl, DriverException e, int nbRetry) {
220+
// do not retry these by default as they generally indicate a data problem or
221+
// other issue that is unlikely to be resolved by a retry.
225222
if (e instanceof WriteFailureException || e instanceof ReadFailureException) {
226223
return RetryDecision.rethrow();
227224
}
228-
return (nbRetry == 0) ? RetryDecision.tryNextHost(cl) : RetryDecision.rethrow();
225+
return RetryDecision.tryNextHost(cl);
229226
}
230227

231228
@Override

0 commit comments

Comments
 (0)