-
Notifications
You must be signed in to change notification settings - Fork 642
CASSGO-4 Support of sending queries to the specific node #1793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -456,6 +456,7 @@ func (s *Session) Query(stmt string, values ...interface{}) *Query { | |
| qry.session = s | ||
| qry.stmt = stmt | ||
| qry.values = values | ||
| qry.hostID = "" | ||
| qry.defaultsFromSession() | ||
| return qry | ||
| } | ||
|
|
@@ -949,6 +950,10 @@ type Query struct { | |
|
|
||
| // routingInfo is a pointer because Query can be copied and copyable struct can't hold a mutex. | ||
| routingInfo *queryRoutingInfo | ||
|
|
||
| // hostID specifies the host on which the query should be executed. | ||
| // If it is empty, then the host is picked by HostSelectionPolicy | ||
| hostID string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's an internal field so we can call it whatever. again, i think that it's obvious enough as-is without "target" but i'm fine internally if there's a need to clarify.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to leave |
||
| } | ||
|
|
||
| type queryRoutingInfo struct { | ||
|
|
@@ -1442,6 +1447,20 @@ func (q *Query) releaseAfterExecution() { | |
| q.decRefCount() | ||
| } | ||
|
|
||
| // SetHostID allows to define the host the query should be executed against. If the | ||
| // host was filtered or otherwise unavailable, then the query will error. If an empty | ||
| // string is sent, the default behavior, using the configured HostSelectionPolicy will | ||
| // be used. A hostID can be obtained from HostInfo.HostID() after calling GetHosts(). | ||
| func (q *Query) SetHostID(hostID string) *Query { | ||
| q.hostID = hostID | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should check if it's a non empty string because if it's empty then the query will be executed as if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. If we add a check then we should panic if it is empty which is probably not the best, but using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing a pointer seems a bit awkward to use and is very unusual given all of the other functions in this package. What happens if you call We should just document that sending an empty string will restore the default behavior.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I spent some time thinking about this more and I'm +1 on what @jameshartig says. We at least should have a way to restore If All of this is based on the case when we want to copy the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I suggested the pointer I wasn't actually saying we should make the parameter a pointer, I was saying we could make the private field a pointer so we know when the user actually set the value or not. The parameter would still be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but yeah I think we can just document it instead of checking and failing. To return an error we would have to change the method signature which would be awkward (or panic'ing but that's just a bad idea anyway)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comments for this method are getting lengthly. How about: // SetHostID allows to define the host the query should be executed against. If the
// host was filtered or otherwise unavailable, then the query will error. If an empty
// string is sent, the default behavior, using the configured HostSelectionPolicy will
// be used. A hostID can be obtained from HostInfo.HostID() after calling GetHosts().I removed the WithContext part because we plan to fix that in a follow-up issue.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a lot. It is much better!
Oh I completely forgot about this, thanks lots! |
||
| return q | ||
| } | ||
|
|
||
| // GetHostID returns id of the host on which query should be executed. | ||
| func (q *Query) GetHostID() string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd name this method
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you like it, then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, they both have better semantics... If we want to be consistence with the same API in other drivers it should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if |
||
| return q.hostID | ||
| } | ||
|
|
||
| // Iter represents an iterator that can be used to iterate over all rows that | ||
| // were returned by a query. The iterator might send additional queries to the | ||
| // database during the iteration if paging was enabled. | ||
|
|
@@ -2057,6 +2076,11 @@ func (b *Batch) releaseAfterExecution() { | |
| // that would race with speculative executions. | ||
| } | ||
|
|
||
| // GetHostID satisfies ExecutableQuery interface but does noop. | ||
| func (b *Batch) GetHostID() string { | ||
| return "" | ||
| } | ||
|
|
||
| type BatchType byte | ||
|
|
||
| const ( | ||
|
|
@@ -2189,6 +2213,11 @@ func (t *traceWriter) Trace(traceId []byte) { | |
| } | ||
| } | ||
|
|
||
| // GetHosts return a list of hosts in the ring the driver knows of. | ||
| func (s *Session) GetHosts() []*HostInfo { | ||
| return s.ring.allHosts() | ||
| } | ||
|
|
||
| type ObservedQuery struct { | ||
| Keyspace string | ||
| Statement string | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option here rather than changing the
ExecutableQueryis to do:then later in
executeQueryyou'd do:Then we wouldn't need to break this interface and worry about modifying batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the first approach I've proposed. We discussed this with @joao-r-reis. If you have an opinion on this, let's discuss it here.
TL;DR
We have two solutions how to avoid type casts and make the implementation more obvious:
ExecutableQueryinterface, because it can't be implemented outside gocql due to private methods likeexecute, so we cannot consider it a breaking change.ExecutableQueryto an internal interface which also should implementExecutableQueryand use it internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think we need to treat
ExecutableQueryas a "public" interface to be implemented by users, it's a bit messy right now and I actually want to spend some time investigating how we could clean up this part of the API as part of #1848 ideally in time for the 2.0 releaseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I was just suggesting so we didn't have to have a weird case for Batch. I think its fine to add and we can aim to improve it in a future MR.