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.
host_source.go
Outdated
| // 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.
cluster.go
Outdated
| // 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.
There was a problem hiding this comment.
Agree, I've returned the old name.
|
If there's no update to this PR soon I'll descope it from the 2.0 release as this can be released in a minor released without an issue |
|
@joao-r-reis I'm going to update it by the end of next week, thanks for the comments! |
|
Hi. I’ve handed this PR over to @tengu-alt, who will finalize the remaining work. I’m switching to another task, so please feel free to tag him if you have any questions or need updates. |
tengu-alt
force-pushed
the
disable-host-lookup
branch
from
6cd5280 to
1d13f9b
Compare
|
Made some updates: refactored tests, added new logic( |
|
@joao-r-reis Could you please take a look? |
CHANGELOG.md
Outdated
|
|
||
| - Fix DisableInitialHostLookup flag ignored when querying system.peers (CASSGO-5) | ||
|
|
||
| - Fix DisableInitialHostLookup flag ignored when querying system.peers (PR update) (CASSGO-5) |
| for time.Now().Before(endDeadline) { | ||
| iter := c.querySystemPeers(ctx, c.host.version) | ||
|
|
||
| versions = make(map[string]struct{}) |
There was a problem hiding this comment.
why this change? resetting the map on each attempt made more sense to me
disable_host_lookup_test.go
Outdated
| "testing" | ||
| ) | ||
|
|
||
| func TestDisableHostLookupRingRefresh(t *testing.T) { |
There was a problem hiding this comment.
can we not add this to an existing test file? Creating a new file just for this test seems excessive. Also this is running in "unit tests" step because it's missing the appropriate build flag at the top of the file. Also missing license header.
There was a problem hiding this comment.
Done, I've moved the test into the cassandra_test.go
|
Also, I'm considering pushing this to a 2.x minor release followup and descoping it from 2.0 because it doesn't really need a major release and I also want do some extensive testing with the |
…PR update) This commit provides update for the 'Modified DisableInitialHostLookup flag name and logic' refreshRing() does NOOP when 'DisableInitialHostLookup' is true. system.peers can be queried when flag is true for the schema version retrieving. patch by Oleksandr Luzhniy; for CASSGO-5
tengu-alt
force-pushed
the
disable-host-lookup
branch
from
1d13f9b to
f5b6f8a
Compare
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.