CASSGO-39 Add query attempt interceptor #1820
Conversation
Removes return statement that bypassed query attempt tracking.
example_interceptor_test.go
Outdated
|
|
||
| // Optionally bypass the handler and return an error to prevent query execution. | ||
| // For example, to simulate query timeouts. | ||
| if q.injectFault && query.Attempts() == 0 { |
There was a problem hiding this comment.
Should the attempt number be a parameter to Intercept? It seems that calling Attempts() could be racy, especially when speculative execution is enabled:
- Attempt 0 started
- Attempt 1 started
- Interceptor 0 called - gets attempt 0
- Interceptor 1 called - gets attempt 0
- attempts incremented
- attempts incremented
Perhaps the signature should be Intercept(ctx context.Context, attempt gocql.QueryAttempt, handler gocql.QueryAttemptHandler)? That would enable adding more information about the attempt in the future.
type QueryAttempt struct {
// query to execute
query gocql.ExecutableQuery
// conn to use to execute the query
conn *gocql.Conn
// number of the attempt. 0 is the initial attempt, 1 is first retry, etc.
number int
}type QueryAttemptHandler = func(context.Context, QueryAttempt) *IterThere was a problem hiding this comment.
@martin-sucha passing a QueryAttempt type seems like a great way to keep the interceptor API stable 👍
query_executor.go
Outdated
|
|
||
| // The interceptor is responsible for calling the `handler` function and returning the handler result. Failure to | ||
| // call the handler will panic. If the interceptor wants to halt query execution and prevent retries, it should | ||
| // return an error. |
There was a problem hiding this comment.
nitpick: The function does not have an error return value. Should we add an error return value or should we reference NewIterWithErr instead?
question: How does returning an error prevent a retry?
There was a problem hiding this comment.
Should we add an error return value or should we reference
NewIterWithErrinstead?
Updated Intercept to optionally return an error and removed NewIterWithErr.
How does returning an error prevent a retry?
Returning a non-retriable error like context.Canceled would prevent retries, but admittedly that's not clear from the function docs. I removed the reference to preventing retries and will assume that users can probably figure out how to configure retry policies with whatever error handling and retry prevention makes sense for them.
Remove gocql.NewIterWithErr
| // The connection used to execute the query. | ||
| Conn *Conn | ||
| // The host that will receive the query. | ||
| Host *HostInfo |
There was a problem hiding this comment.
Do we need to add and expose these fields in QueryAttempt?
If there’s no compelling need for direct connection manipulation, then maybe we could expose only a connection metadata that’s relevant for logging or monitoring purposes?
| err = session.Query("select now() from system.local"). | ||
| WithContext(ctx). | ||
| RetryPolicy(&gocql.SimpleRetryPolicy{NumRetries: 2}). | ||
| Scan(&stringValue) | ||
| if err != nil { |
There was a problem hiding this comment.
How about adding an option to apply the interceptor selectively to specific queries, like .WithInterceptor() method to the Query struct?
|
I don't know if I like the idea of having an |
|
Rate limiting should probably also be handled in a different component with its own interface dedicated to rate limiting/throttling |
|
Hmm taking another look at #1786 it seems that the focus is indeed to be an interface generic enough to serve as a way to implement stuff like request limiting or query modification while the inspection stuff could be handled by the observers that we already have. In that case we need to make sure the changes here are consistent with changes in #1447 |
joao-r-reis
changed the title
As suggested in #1786, this PR introduces a
QueryAttemptInterceptorinterface for intercepting query attempts, which allows clients to inject logic that should apply to all queries e.g. context manipulation, rate limiting, query modification, logging, fault injection, metrics, etc.Alternative to #1810.