absurdfarce commented on PR #1905: URL: https://github.com/apache/cassandra-java-driver/pull/1905#issuecomment-1906652412
Note that the underlying issue in JAVA-3157 isn't really a throttler question at all; more on that [here](https://datastax-oss.atlassian.net/browse/JAVA-3157). But we can still consider this change in isolation. My main concern here is that this change might hide errors in the (highly unlikely) case that there's a problem registering a throttler. CqlRequstHandler calls throttler.register() in it's constructor; as @aratno points out above if that fails we then call CqlRequestHandler.onThrottleFailure() which updates some metrics and then sets the final error on the underlying CompletableFuture. Sesms like we should avoid the metrics operation in this case; this is a failure to register a throttler not a case of throttling. Most staightforward thing may be to add Throttled.onRegisterFailure() (or something similar). The implementation of that method can then just set the final state without updating session metrics. Might also want to use a more specific type than RequestThrottlingException for onRegisterFailure(), although if we already have discrete methods to distinguish between an actual throttler failure and a failure to register a throttler then perhaps a new type doesn't buy us much. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
