-
Notifications
You must be signed in to change notification settings - Fork 639
CASSGO-72 Fix connection issue in Amazon Keyspaces #1886
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
base: trunk
Are you sure you want to change the base?
Conversation
host_source.go
Outdated
hostMap := make(map[string]*HostInfo, len(hosts)) | ||
for _, host := range hosts { | ||
hostMap[host.HostID()] = host | ||
} | ||
|
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.
I don't think this solution is straighforward, it relays on the fact that r.GetHosts()
provides hosts in the certain order, if that order changes it stop working.
It would be better to update GetHosts
to ignore hosts that are already in the list
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.
Thank you for reviewing. I will try to fix GetHosts()
.
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.
I fixed GetHosts()
to ignore localHost if the same Host ID exists.
Could you please confirm?
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.
looks good, can you please add comment why this code is there, mentioning issue number and Amazon Keyspaces
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.
Yes, I added the comment.
In Amazon Keyspaces, system.local returns the localhost address and system.peers returns a host that contains the same hostID as localhost. This causes the connection issue in refreshRing(), where host address is overwritten with the localhost address and a host with the same hostID results in the "cannot find host" error. This commit fixes GetHosts() so that it ignores localhost if the same hsotID is included in peerHosts.
This needs a test. Also, this addresses one of the issues but doesn't address the issue with the control connection querying the local table and getting back the wrong information. See #1873 (comment) for more details. I was going to suggest that we do what that comment suggests and call The problem with |
} else { | ||
hosts = append([]*HostInfo{localHost}, peerHosts...) | ||
} | ||
|
||
var partitioner string | ||
if len(hosts) > 0 { | ||
partitioner = hosts[0].Partitioner() |
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.
I think this needs to instead look at localHost
instead because I don't think the peers table contains partitioner information.
Yeah the fix is to make sure we do a full ring refresh instead of just updating the local host so that we can overwrite the local host info with the info from system.peers. Doing a full refresh even without any overwriting would also mitigate the issue because the current behavior of every host turning into 127.0.0.1 eventually after enough reconnects would no longer exist. |
The problem is that |
Thank you for comments. I'm afraid but I don't understand the details for now. I will look into your comments and the codes. |
This PR fixes #1873
In Amazon Keyspaces, system.local returns the localhost address and system.peers returns a host that contains the same hostID as localhost. This causes the connection issue in refreshRing(), where host address is overwritten with the localhost address and a host with the same hostID results in the "cannot find host" error.
This commit fixes the issue by making map from a slice of hosts. This approach is also used in
func (s *Session) init()
https://github.com/apache/cassandra-gocql-driver/blob/trunk/session.go#L272-L275