Skip to content
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

CASSGO-4 Support of sending queries to the specific node #1793

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

worryg0d
Copy link
Contributor

@worryg0d worryg0d commented Aug 5, 2024

Overview

This PR provides a mechanism that allows users to specify on which node the query will be executed. It is not a typical use case, but it makes sense with virtual tables.
For example, when we want to retrieve metrics for a specific node, we have to send queries to the associated system view of this node.

Implementation Overview

A new method SetHost() SetHostID for the Query allows users to specify the host (node) on which the driver must send the query. It is implemented by adding a GetHost() to the ExecutableQuery interface. When the host is specified for the query the driver ignores any retry, speculative, and host selection policies.

Usage Example

Here is an example of retrieving metrics from system_view.cql_metrics for each node of the cluster:

cluster := NewCluster("127.0.0.1")
session, err := cluster.CreateSession()
if err != nil {
	panic(err)
}
defer session.Close()

hosts, err := session.GetHosts()
if err != nil {
	panic(err)
}

type cqlMetric struct {
	name  string
	value float64
}

// hostId to []cqlMetric
cqlMetrics := map[string][]cqlMetric{}

for _, host := range hosts {
	iter := session.Query("SELECT * FROM system_views.cql_metrics").
		SetHostID(host.HostID()).
		Iter()

	metrics := make([]cqlMetric, 0, iter.NumRows())
	metric := cqlMetric{}
	for iter.Scan(&metric.name, &metric.value) {
		metrics = append(metrics, metric)
	}
	cqlMetrics[host.HostID()] = metrics
}

@worryg0d worryg0d force-pushed the query-to-specific-node branch from b2f7e6f to 0444ee3 Compare August 5, 2024 08:50
@martin-sucha
Copy link
Contributor

Should the API allow for other routing preferences? For example only a single data center/rack? Or be extensible to allow targeting a specific shard in the scylladb/gocql fork?

Should the API allow to route to a different host during retry, i.e. allow passing a HostSelectionPolicy or a similar interface/function?

@worryg0d
Copy link
Contributor Author

worryg0d commented Aug 6, 2024

Should the API allow for other routing preferences? For example only a single data center/rack? Or be extensible to allow targeting a specific shard in the scylladb/gocql fork?

Should the API allow to route to a different host during retry, i.e. allow passing a HostSelectionPolicy or a similar interface/function?

HostInfo provides API to retrieve information like dc or rack for the host, so users may filter retrieved hosts manually before binding them to the query.

In my opinion, re-routing queries bound to the specific hosts to others is not a good idea even during retries, at least as default behavior. Sending queries to a specific host is not a typical use case and if we try to do it, we assume that the specified host is reachable. Or, at least, we expect that the query was attempted to be sent to the specified host.

The idea of this API being extensible sounds good, but I am not sure about implementation. May you have some advice or ideas on how to properly implement this?

@joao-r-reis joao-r-reis changed the title Support of sending queries to the specific node CASSGO-4 Support of sending queries to the specific node Oct 29, 2024
@joao-r-reis
Copy link
Contributor

I think keeping this focused on the virtual table use case is best. Other routing preferences like shard awareness for scylla should probably be handled at the HostSelectionPolicy level. For this, we could add a function on Query and Batch that allows the HostSelectionPolicy to be provided instead of using the session level one but this can be handled on a separate JIRA and PR, it's a different use case than this one which is focused on virtual table queries that should be strongly associated with a specific host.

If a user sets the host using Query.SetHost() then either we disable retries and speculative executions altogether for that query or we allow retries and speculative executions only on that host (basically the host selection policy should never be used in the context of this query).

In the future if we add a way to provide a HostSelectionPolicy in addition to .SetHost() then the driver should probably return an error saying that you can't simultaneously set a host and provide a policy on the same query.

@joao-r-reis
Copy link
Contributor

When the queryExecutor.executeQuery is called then it checks if the provided ExecutableQuery implements the hostGetter interface as well. We need this interface to not change the API ExecutableQuery. Also, the ExecutableQuery is implemented by Query and Batch as well, so it is good to avoid adding redundant methods for Batch as well.

So, if provided ExecutableQuery implements hostGetter, then it type-asserts it and calls the underlying method to get the host. If the host is not nil, it wraps the host into hostIter func which just returns the specified host.

The discussion around the ExecutableQuery interface and the Query API in general probably has a decent overlap with the work being dicussed and implemented in https://issues.apache.org/jira/browse/CASSGO-22 , justfyi.

@@ -83,7 +83,28 @@ func (q *queryExecutor) speculate(ctx context.Context, qry ExecutableQuery, sp S
}

func (q *queryExecutor) executeQuery(qry ExecutableQuery) (*Iter, error) {
hostIter := q.policy.Pick(qry)
type hostGetter interface {
Copy link
Contributor

@joao-r-reis joao-r-reis Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a private interface like:

type cqlQuery interface {
    ExecutableQuery
    getHost() *HostInfo
}

And make gocql internal code reference cqlQuery (name subject to change?) instead of ExecutableQuery so that we don't have to worry about breaking the public ExecutableQuery interface when we add new functions to Query or Batch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could just add new functions to ExecutableQuery because there's no place where a user can provide a custom implementation of ExecutableQuery to the driver is there? If the driver API doesn't expose any function that receives an object of type ExecutableQuery then we should be fine changing this interface since there's no real use case where a user might implement it on their application.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick glance at the code, ExecutableQuery is provided to implementations of HostSelectionPolicy but it's the driver that calls HostSelectionPolicy.Pick(ExecutableQuery) with Query or Batch objects as parameters so I don't see an issue in adding GetHost to ExecutableQuery

Copy link
Contributor Author

@worryg0d worryg0d Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind extending ExecutableQuery interface, but adding private methods is not the best thing to me. The driver exposes HostSelectionPolicy interface and allows user to provide their own implementations, but users cannot properly test them because Pick() accepts ExecutableQuery that cannot be mocked for testing purposes due to private methods inside... Ofc they can just pass Query and Batch objects, but I'm unsure if it is enough for testing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These private methods would not be used in HostSelectionPolicy because HostSelectionPolicy declares the parameter as ExecutableQuery not cqlQuery or whatever the name would be for the private interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what private methods are you referencing?

Such methods as execute(), attempt(), etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right, I didn't notice them. 👍 So it wouldn't actually make much of a difference if we added a private getHosts method there but we can keep it public

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only issue of making GetHost() public is a requirement to implement it for Batch only to satisfy the interface. So there will be a public API Batch.GetHost() which actually does noop. Maybe the first approach with internal query interface for internal use and the public one for exposing is better...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine if Batch.GetHost() does a noop honestly, there's also a possibility that we might make it possible to set the host on batch queries in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's leave this one as it is for now

@worryg0d worryg0d force-pushed the query-to-specific-node branch 2 times, most recently from d8ebae3 to c1e46be Compare November 21, 2024 11:25

var lastErr error
var iter *Iter
for selectedHost != nil {
host := selectedHost.Info()
if host == nil || !host.IsUp() {
if (host == nil || !host.IsUp()) && specifiedHost == nil {
selectedHost = hostIter()
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably add another if here to exit right away if specified host is != nil and the host is down

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense not to send the query if the host is down. I added a check before this if that immediately returns an ErrNoConnections when the specified host is down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@worryg0d worryg0d force-pushed the query-to-specific-node branch from c1e46be to d83e8a2 Compare November 21, 2024 16:10
@worryg0d worryg0d force-pushed the query-to-specific-node branch 2 times, most recently from 33730fa to 63c6f5d Compare November 26, 2024 09:55
@worryg0d
Copy link
Contributor Author

Force pushed due to conflicts with the trunk

@jameshartig
Copy link
Contributor

Once we introduce a new API it's very hard to change/remove it. Do we really think that accepting a *HostInfo is the right argument for SetHost? If you already had a host id there's no way for a user to construct a *HostInfo.

@worryg0d
Copy link
Contributor Author

Hello @jameshartig, thanks a lot for your feedback.

If you already had a host id there's no way for a user to construct a *HostInfo.

You have a point, I really forgot that we can't easily create HostInfo obj outside the driver. Then I see two ways how to solve it:

  1. Add NewHostInfo(hostId string) constructor. It is the easiest solution and hostId is enough to get conn pool from queryExecutor.pool.
  2. Introduce a new type Host which will mostly duplicate HostInfo with public fields. We can't make the fields of HostInfo public because they're protected by the mutex. With a new type users will able to easily construct Host and users may easily get access to the host metadata with session.GetHosts() ([]Host, err).

Honestly, I'd prefer the first one because with the second we'll have two Host and HostInfo types that both hold the same fields, but the one is used for internal and can't be created outside gocql and another that is mostly the same but is not used internally...

@jameshartig
Copy link
Contributor

  1. Add NewHostInfo(hostId string) constructor. It is the easiest solution and hostId is enough to get conn pool from queryExecutor.pool

Is there a reason we wouldn't just make SetHost accept a hostId instead? That seems simpler? It would be easy to call with *HostInfo and you don't need a *HostInfo if you don't have one.

@worryg0d
Copy link
Contributor Author

Is there a reason we wouldn't just make SetHost accept a hostId instead?

Honestly, I didn't think about it... I was refering to how such API is implemented in java driver, but we don't have to implement it the same way. I don't mind to reimplement it to use string instead.

@joao-r-reis What do you think about this? You've already approved this PR but we're going to change the implementation so we should agree on the proposal

@joao-r-reis
Copy link
Contributor

Honestly I'm fine with either, on one hand providing a host struct is a bit more consistent with other drivers I think but on the other hand making the parameter a host id seems simpler to use from the user's POV so if James prefers the host id approach then +1 for that one

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Jan 29, 2025

I will say that making it a string will make it more error prone because the user might use the wrong string format of the uuid. This will not be an issue if the user just calls hostInfo.HostID() but at that point might as well just make the parameter a *hostInfo instead. Wdyt about this @jameshartig

@jameshartig
Copy link
Contributor

I will say that making it a string will make it more error prone because the user might use the wrong string format of the uuid. This will not be an issue if the user just calls hostInfo.HostID() but at that point might as well just make the parameter a *hostInfo instead. Wdyt about this @jameshartig

Hmm that is a good point, but by only accepting a *HostInfo the only way to use it is by calling GetHosts and then looping (and optionally filtering) yourself. That's not the end of the world but it does seem less than ideal.

@jameshartig
Copy link
Contributor

@worryg0d Actually thinking about it more, I dislike that GetHosts requires 2 round-trips, and that seems excessive to always require when you want to choose a host, especially if you already have the HostId. I don't think we should be encouraging people to hold onto *HostInfo's which they might do if the only way to get one is calling GetHosts.

I'm doubling down on making it accept a HostId string and we can just look in the pool for that hostId and not need to do any round-trips.

We should mention in the method comment for SetHost that it's recommended to get the hostId from *HostInfo.HostID() to encourage people use it but it's not required.

@worryg0d
Copy link
Contributor Author

worryg0d commented Jan 29, 2025

OK, I'll update this PR tomorrow. Once we're going to change it to take the host id then should it be something like SetHostID(hostID string)?. I am fine with both variants, but the second one has better semantics here

@jameshartig
Copy link
Contributor

OK, I'll update this PR tomorrow. Once we're going to change it to take the host id then should it be something like SetHostID(hostID string)?. I am fine with both variants, but the second one has better semantics here

I like SetHostID because it leaves open the ability to add SetHost later if we needed to.

@worryg0d
Copy link
Contributor Author

I pushed 73590c2 which changes of SetHost() -> SetHostID(string).

@jameshartig @joao-r-reis Could you please take a look at this? Thanks a lot!

// if the specified host goes down during execution, the driver will try to send the query to this host until it succeeds
// which may lead to an endless execution.
func (q *Query) SetHostID(hostID string) *Query {
q.hostID = hostID
Copy link
Contributor

Choose a reason for hiding this comment

The 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 SetHostID() was never called which is odd behavior. Either that or we change q.hostID to be *string so we can check if it was actually set or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 *string seems to be fine

Copy link
Contributor

Choose a reason for hiding this comment

The 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 RoutingKey(nil) or PageState(nil), etc. Setting it to empty could be a way to unset an existing HostID if one was set for some reason prior. When queries are immutable it could be useful to get a copy of a query without a specific HostID that previously had one set.

We should just document that sending an empty string will restore the default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Query.hostID to default.

If SetHostID takes *string it is available, but this makes UX of this API way worse IMO. I don't like the idea of creating vars to hold some strings and pass a pointer to them. However, if API is SetHostID(string) there is no way to restore its behavior to default. Once SetHostID() is used, it can't be changed. Ofc we can expose API Query.Default() to restore this behavior, but this becomes something very odd to me.

All of this is based on the case when we want to copy the existing Query and somehow modify it, but I'm unsure how this matches query immutability. By this do we understand that once the Query obj is created it can't be modified? Either it means that the driver doesn't modify Query internally during its execution? As far as I know, gocql writes some metrics to the query obj.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 string.

Copy link
Contributor

@joao-r-reis joao-r-reis Jan 31, 2025

Choose a reason for hiding this comment

The 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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If HostFilter is causing issues, consider mentioning it in the documentation. For example: 'SetHostID will not work on filtered nodes (i.e., nodes excluded by ClusterConfig.HostFilter)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. It is much better!

I removed the WithContext

Oh I completely forgot about this, thanks lots!

session.go Outdated
@@ -2177,6 +2203,15 @@ func (t *traceWriter) Trace(traceId []byte) {
}
}

// GetHosts returns a list of hosts found via queries to system.local and system.peers
func (s *Session) GetHosts() ([]*HostInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have no way of getting the hosts currently?

Also, we should document that this method performs 2 round trips, some users might be used to other drivers caching the hosts list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have no way of getting the hosts currently?

As I remember the driver doesn't expose API for this purpose. Users can get this via policies like HostSelectionPolicy, but directly no.

some users might be used to other drivers caching the hosts list

I think more about how we can avoid 2 round trips and we can probably take known for the driver hosts from the pool Session.executor.pool.hostConnPools. Each pool contains a reference to HostInfo for which it is created.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the pool since it's filtered by the hostSelectionPolicy. We can use the session's ring.allHosts() method though.

Copy link
Contributor Author

@worryg0d worryg0d Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize ring.allHosts() exists. On any node events ring is updated so ring.allHosts() will return updated hosts. Thanks a lot 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use ring.allHosts(). However, I'm unsure if the comment I wrote is enough... ring.allHosts() returns only hosts known by the driver, which means if ClusterConfig.HostFilter is presented, ring won't have filtered hosts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we could call it GetAllHosts to clarify that it will get all hosts but I'm not sure if that's strictly necessarily. We could call the host filter within the method when we're looping but tbh I'm not sure if users want it filtered or not. I think until we get feedback we just need to pick a path and be prepared to add another method (either add GetAllHosts later or GetHosts later that does filtering). @joao-r-reis do you have thoughts on the filtering vs not?

}
hostIter = func() SelectedHost {
// forcing hostIter to always return the same host
// it makes any retries and speculative executions run on the specified host
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just prevent retries and speculative executions when the host is set? Having potentially endless executions on the same host doesn't look right to me.

Copy link
Contributor Author

@worryg0d worryg0d Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having potentially endless executions on the same host doesn't look right to me

Yea I don't like this one too. It makes sense to prevent speculative executions when we use this API, but disabling retry policies... Probably it makes sense once we use it we assume that the host is reachable.

However, providing context via WithContext() will prevent endless executions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think retries makes sense. That can always be overwritten if they don't want that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more worried that someone might run into endless executions in production, face some kind of outage and only then realize that they need to provide context to ensure the goroutine doesn't get stuck on a potentially infinite loop. But I think in most cases the retry policy will have a max number of retries anyways so that's good enough for me, let's just make sure this is documented in the SetHostID method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at queryExecutor.do more properly and discovered that context cancelation indeed won't protect users from endless executions here.

On line 158 will continue loop iteration if the specified host is down, so it never reaches 179.

This is not a big problem if we're going to disable retries when the SetHostID API is used. For this case hostIter should return the specified host only once. Either queryExecutor.do should be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent more time on this and found a way to avoid endless executions when SetHostID() is used with the downed host. I pushed in on my other branch here. Could you please take a look and share your thoughts?

Ofc this issue is still relevant when HostSelectionPolicy returns downed nodes. At least, this will resolve the described issue with SetHostID(). Thanks a lot to @ribaraka who pointed to this approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that approach is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @jameshartig doesn't have any concerns about the proposed change I'll update this PR

@@ -166,6 +187,8 @@ func (q *queryExecutor) do(ctx context.Context, qry ExecutableQuery, hostIter Ne

// Exit if the query was successful
// or query is not idempotent or no retry policy defined
// Also, if there is specified host for the query to be executed on
// and query execution is failed we should exit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this comment added here? Is this the relevant place for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is left from the previous diff when it used *HostInfo and it should be removed. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 🙌

@@ -41,6 +41,7 @@ type ExecutableQuery interface {
Keyspace() string
Table() string
IsIdempotent() bool
GetHostID() string
Copy link
Contributor

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 ExecutableQuery is to do:

type hostIDQuery interface {
  GetHostID() string
}

then later in executeQuery you'd do:

var hostID string
if hostIDQry, ok := qry.(hostIDQuery); ok {
  hostID = hostIDQry.GetHostID()
}

Then we wouldn't need to break this interface and worry about modifying batch.

Copy link
Contributor Author

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:

  1. Extend ExecutableQuery interface, because it can't be implemented outside gocql due to private methods like execute, so we cannot consider it a breaking change.
  2. Extract private methods from ExecutableQuery to an internal interface which also should implement ExecutableQuery and use it internally.

Copy link
Contributor

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 ExecutableQuery as 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 release

Copy link
Contributor

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.

}
hostIter = func() SelectedHost {
// forcing hostIter to always return the same host
// it makes any retries and speculative executions run on the specified host
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think retries makes sense. That can always be overwritten if they don't want that.

// if the specified host goes down during execution, the driver will try to send the query to this host until it succeeds
// which may lead to an endless execution.
func (q *Query) SetHostID(hostID string) *Query {
q.hostID = hostID
Copy link
Contributor

Choose a reason for hiding this comment

The 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 RoutingKey(nil) or PageState(nil), etc. Setting it to empty could be a way to unset an existing HostID if one was set for some reason prior. When queries are immutable it could be useful to get a copy of a query without a specific HostID that previously had one set.

We should just document that sending an empty string will restore the default behavior.

session.go Outdated
@@ -2177,6 +2203,15 @@ func (t *traceWriter) Trace(traceId []byte) {
}
}

// GetHosts returns a list of hosts found via queries to system.local and system.peers
func (s *Session) GetHosts() ([]*HostInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the pool since it's filtered by the hostSelectionPolicy. We can use the session's ring.allHosts() method though.

@worryg0d worryg0d force-pushed the query-to-specific-node branch from 5f1f400 to 2aac3e7 Compare January 31, 2025 15:44
@worryg0d
Copy link
Contributor Author

Rebased on trunk and force pushed due to a weird cancellation of the workflow run.

}

// GetHostID returns id of the host on which query should be executed.
func (q *Query) GetHostID() string {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this method GetTargetHostId to make it more specific.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like it, then SetTargetHostID might be changed as well 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 SetHost(), but we don't really have to make the same...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if Target is specifically necessary since it seems kind of obvious what the point of the host is. I do agree that staying closer to the java API is probably ideal, which SetHostID tries to do.


// hostID specifies the host on which the query should be executed.
// If it is empty, then the host is picked by HostSelectionPolicy
hostID string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about queryTargetHostId or targetHostID?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to leave hostID to be consistent with method names

@@ -3347,3 +3346,53 @@ func TestQuery_NamedValues(t *testing.T) {
t.Fatal(err)
}
}

func TestQuery_SetHostID(t *testing.T) {
// This test ensures that queries are sent to the specified host only
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: It might be better to keep comments above the test function to clearly describe its overall purpose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

// if the specified host goes down during execution, the driver will try to send the query to this host until it succeeds
// which may lead to an endless execution.
func (q *Query) SetHostID(hostID string) *Query {
q.hostID = hostID
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If HostFilter is causing issues, consider mentioning it in the documentation. For example: 'SetHostID will not work on filtered nodes (i.e., nodes excluded by ClusterConfig.HostFilter)

// if the specified host goes down during execution, the driver will try to send the query to this host until it succeeds
// which may lead to an endless execution.
func (q *Query) SetHostID(hostID string) *Query {
q.hostID = hostID
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@worryg0d worryg0d requested a review from jameshartig February 7, 2025 11:30
@jameshartig
Copy link
Contributor

@worryg0d can you squash these commits and update the commit message to add my name to approvers

Query.SetHost() allows users to specify on which node the Query will be executed.
It is not a tipycal use case, but it makes sense with virtual tables which are available since C* 4.0.

Patch by Bohdan Siryk; Reviewed by João Reis, James Hartig for CASSGO-4
@worryg0d worryg0d force-pushed the query-to-specific-node branch from bd41e2d to 625e0de Compare February 18, 2025 16:17
@worryg0d
Copy link
Contributor Author

@jameshartig I squashed commits and updated the message. Before merging it to the main could you please take a look at this? There is an approach to avoid deadlocks when a downed host is selected by SetHostID().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants