CASSGO-5 Fix DisableInitialHostLookup flag ignored when querying system.peers #1790
Conversation
joao-r-reis
changed the title
There was a problem hiding this comment.
Added some comments.
In summary I think this PR should be limited to making the refreshRing function do nothing when the flag is set. That would ensure that the driver will keep using the contact points as the hosts forever and ignore any kind of topology changes or any host information from system tables.
It's fine if the driver queries the system tables to check if there is schema agreement, I don't think it goes against the intention of the flag.
|
|
||
| if err = iter.Close(); err != nil { | ||
| goto cont | ||
| } |
There was a problem hiding this comment.
I think it's fine if we query system.peers for the purpose of retrieving the schema version of each host as long as we don't update gocql's ring/hosts. If a user wants to use a static set of contact points and avoid using hosts from system tables there's no reason why they would have to give up the functionality of awaiting for schema agreement.
| // Check if host lookup is disabled | ||
| if r.session.cfg.DisableHostLookup { | ||
| return []*HostInfo{}, nil | ||
| } |
There was a problem hiding this comment.
Instead of adding this, I think we should make the ringRefresher do a NOOP when this flag is set. getClusterPeerInfo should only be about retriving the peers info from the server so it should be up to the caller whether this should be done or not.
If we make the func refreshRing(r *ringDescriber) error function do nothing when the flag is set then it should solve all of the issues related to the driver not respecting the flag.
| // from the system.peers table, this will mean that the driver will connect to | ||
| // hosts supplied and will not attempt to lookup the hosts information, this will | ||
| // mean that data_centre, rack and token information will not be available and as | ||
| // such host filtering and token aware query routing will not be available. | ||
| DisableInitialHostLookup bool | ||
| DisableHostLookup bool |
There was a problem hiding this comment.
I don't think this is worth breaking applications. I agree that removing the Initial word from the name does make sense but it's really not that big of a deal.
There was a problem hiding this comment.
At the very least we would have to keep both flags (+ deprecate the old one) and map it to a single internal flag so that we can do a "safe" rename of the flag without breaking user applications.
This PR addresses issue #1665, which reports a bug where
system.peerstable is queried even ifDisableInitHostLookupflag is set totrue. According to the driver documentation,system.peersshould not be queried when the flag is set totrueIn this PR
DisableInitialHostLookupflag renamed toDisableHostLookup, code logic is fixed to querysystem.peersonly when the flag is set tofalse.