toptobes commented on PR #1943:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1943#issuecomment-5008778135
Hi folks, I'm just doing an initial pass-through on behalf of Joao to
provide some initial feedback before he gets a chance to look at it.
I'm not super experienced with the gocql driver itself, but from a purely Go
point-of-view, it looks pretty solid. There's just a few relatively minor
things I'll note in separate, dedicated comments.
One of my major takeaways from this though, was that exec intercepters could
effectively replace `[Batch|Query]Observer`s with a little work, for the sake
of keeping the API simple for users, rather than having two close/competing APIs
- To be clear, I'm not talking about deleting `[Batch|Query]Observer`, I'm
just suggesting we could somewhat deprecate them or otherwise put them on the
back burner, especially as it's marked w/ an `// Experimental, this interface
and use may change` comment. We could fairly easily just keep them and actually
reimplement them under the hood as exec interceptors so we don't need to
duplicate related code/mechanisms
**Now to be clear, I'm just throwing this out there as an idea, we can
discuss further specifics if we like it; we can trash it instantly if we
don't.**
One way of doing so would be to make `Intercept` return a
`(ExecAttemptResult, error)` instead of `(*Iter, error)` where there's a few
different ways you can implement `ExecAttemptResult`, but a simple version
could be something like:
```go
type ExecAttemptResult struct {
Iter *Iter
q *internalQuery
b *internalBatch
}
// lazy constructed for performance since they're not always needed
// using the existing ObservedQuery & ObservedBatch for easy migration &
compatability
// these could also take a `e *ExecAttemptResult` to cache the results
func (e ExecAttemptResult) GetObservedQuery() ObservedQuery { ... }
func (e ExecAttemptResult) GetObservedBatch() ObservedBatch { ... }
```
Or you could just have the info directly in the `ExecAttemptResult` instead
of using the `Observed[Query|Batch]` types. Whatever works. The main idea is to
stay lazy so a price isn't paid on the hot path if it's not needed.
Then you'd just need to
- Allow setting exec interceptors on a per-query basis (potentially
auto-chaining them? though that could be unexpected)
- Figure out a heuristic for when to use `newHostMetricsManager` (maybe
check if the interceptor satisfies an interface like `interface {
RequiresHostMetrics() bool }` + returns `true`)
- Migrate observers to be interceptors under the hood for simplicity
Maybe there's a few other small things you'd need to do, but I don't think
I'm missing anything major.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
