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-6 'Unable to discover cluster nodes with an empty rack name' issue fix #1785

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

Conversation

tengu-alt
Copy link
Contributor

@tengu-alt tengu-alt commented Jul 24, 2024

This PR addresses issue #1706.

The driver was unable to discover nodes with an empty rack name because the isValidPeer() function incorrectly checked for an empty string in the rack value (which is a valid case). The correct validation should ensure the rack name is not null (as a null value indicates an invalid peer).

This PR fixes the method of retrieving the rack value from the system.peers and system.peers_v2 tables. Now, the rack value is set as a pointer to a string, allowing the value to be checked for null.

@joao-r-reis joao-r-reis changed the title 'Unable to discover cluster nodes with an empty rack name' issue fix CASSGO-6 'Unable to discover cluster nodes with an empty rack name' issue fix Oct 29, 2024
if err != nil {
return RowData{}, err
}
if column.Name == "rack" && column.Keyspace == "system" && (column.Table == "peers_v2" || column.Table == "peers") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a hack, plus it could break backwards compatibility for existing users of RowData (outside of gocql) that read from those tables.
Could the issue be fixed another way, for example improving the API/adding new API for all users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is #1834, which allows to handle a nullable value. After #1834 is merged, I will refactor this PR using the nullable scan functional. This should help to avoid the hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with addressing #1834 first, @martin-sucha can you provide feedback on #1834 ? I see your original comment on that issue, but I'm not sure if #1834 accurately implements what you were proposing.

@@ -157,6 +157,7 @@ type HostInfo struct {
state nodeState
schemaVersion string
tokens []string
isRackNil bool
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 change rack to be *string, it's a private field. A conversion has to happen in the Rack() function though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!
Let's wait for the feedback that you have asked for.

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.

3 participants