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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Change Batch API to be consistent with Query() (CASSGO-7)

- Unable to discover cluster nodes with an empty rack name (CASSGO-6)

### Fixed

- Retry policy now takes into account query idempotency (CASSGO-27)
Expand Down
57 changes: 57 additions & 0 deletions cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
package gocql

import (
"errors"
"net"
"reflect"
"testing"
Expand Down Expand Up @@ -80,3 +81,59 @@ func TestClusterConfig_translateAddressAndPort_Success(t *testing.T) {
assertTrue(t, "translated address", net.ParseIP("10.10.10.10").Equal(newAddr))
assertEqual(t, "translated port", 5432, newPort)
}

func TestEmptyRack(t *testing.T) {
s := &Session{}
host := &HostInfo{}

row := make(map[string]interface{})

row["preferred_ip"] = "172.3.0.2"
row["rpc_address"] = "172.3.0.2"
row["host_id"] = UUIDFromTime(time.Now())
row["data_center"] = "dc1"
row["tokens"] = []string{"t1", "t2"}
row["rack"] = "rack1"

validHost, err := s.hostInfoFromMap(row, host)
if err != nil {
t.Fatal(err)
}
if !isValidPeer(validHost) {
t.Fatal(errors.New("expected valid host"))
}

row["rack"] = ""

validHost, err = s.hostInfoFromMap(row, host)
if err != nil {
t.Fatal(err)
}
if !isValidPeer(validHost) {
t.Fatal(errors.New("expected valid host"))
}

strPtr := new(string)
*strPtr = "rack"
row["rack"] = strPtr

validHost, err = s.hostInfoFromMap(row, host)
if err != nil {
t.Fatal(err)
}
if !isValidPeer(validHost) {
t.Fatal(errors.New("expected valid host"))
}

strPtr = new(string)
strPtr = nil
row["rack"] = strPtr

validHost, err = s.hostInfoFromMap(row, host)
if err != nil {
t.Fatal(err)
}
if isValidPeer(validHost) {
t.Fatal(errors.New("expected valid host"))
}
}
24 changes: 15 additions & 9 deletions helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,21 +331,27 @@ func (iter *Iter) RowData() (RowData, error) {
values := make([]interface{}, 0, len(iter.Columns()))

for _, column := range iter.Columns() {
if c, ok := column.TypeInfo.(TupleTypeInfo); !ok {
val, err := column.TypeInfo.NewWithError()
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.

var strPtr = new(string)
columns = append(columns, column.Name)
values = append(values, val)
values = append(values, &strPtr)
} else {
for i, elem := range c.Elems {
columns = append(columns, TupleColumnName(column.Name, i))
val, err := elem.NewWithError()
if c, ok := column.TypeInfo.(TupleTypeInfo); !ok {
val, err := column.TypeInfo.NewWithError()
if err != nil {
return RowData{}, err
}
columns = append(columns, column.Name)
values = append(values, val)
} else {
for i, elem := range c.Elems {
columns = append(columns, TupleColumnName(column.Name, i))
val, err := elem.NewWithError()
if err != nil {
return RowData{}, err
}
values = append(values, val)
}
}
}
}
Expand Down
16 changes: 13 additions & 3 deletions host_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

func (h *HostInfo) Equal(host *HostInfo) bool {
Expand Down Expand Up @@ -484,9 +485,18 @@ func (s *Session) hostInfoFromMap(row map[string]interface{}, host *HostInfo) (*
return nil, fmt.Errorf(assertErrorMsg, "data_center")
}
case "rack":
host.rack, ok = value.(string)
rack, ok := value.(*string)
if !ok {
return nil, fmt.Errorf(assertErrorMsg, "rack")
host.rack, ok = value.(string)
if !ok {
return nil, fmt.Errorf(assertErrorMsg, "rack")
}
} else {
if rack != nil {
host.rack = *rack
} else {
host.isRackNil = true
}
}
case "host_id":
hostId, ok := value.(UUID)
Expand Down Expand Up @@ -673,7 +683,7 @@ func isValidPeer(host *HostInfo) bool {
return !(len(host.RPCAddress()) == 0 ||
host.hostId == "" ||
host.dataCenter == "" ||
host.rack == "" ||
host.isRackNil ||
len(host.tokens) == 0)
}

Expand Down
Loading