joao-r-reis commented on code in PR #1858:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1858#discussion_r1915101813
##########
control.go:
##########
@@ -261,16 +261,21 @@ func (c *controlConn) connect(hosts []*HostInfo) error {
var conn *Conn
var err error
for _, host := range hosts {
+ connAddr, err := host.ConnectAddress()
+ if err != nil {
+ c.session.logger.Printf("gocql: %v\n", err)
Review Comment:
Usually we add a message for this error before we add the actual error
string so something like:
`c.session.logger.Printf("gocql: unable to use host for control connection,
skipping it: %v\n", err)`
##########
control_test.go:
##########
@@ -49,8 +49,13 @@ func TestHostInfo_Lookup(t *testing.T) {
}
host := hosts[0]
- if !host.ConnectAddress().Equal(test.ip) {
- t.Errorf("expected ip %v got %v for addr %q", test.ip,
host.ConnectAddress(), test.addr)
+ connAddr, err := host.ConnectAddress()
+ if err != nil {
+ t.Errorf("%d: %v", i, err)
Review Comment:
`t.Errorf("could not get connect address of host %q to compare with
expected: %v", test.addr, err)`
##########
filters.go:
##########
@@ -72,10 +72,13 @@ func WhiteListHostFilter(hosts ...string) HostFilter {
m := make(map[string]bool, len(hostInfos))
for _, host := range hostInfos {
- m[host.ConnectAddress().String()] = true
+ connAddr, _ := host.ConnectAddress()
Review Comment:
if `err != nil` I don't think it should be whitelisted, it should just
`continue`
##########
filters.go:
##########
@@ -72,10 +72,13 @@ func WhiteListHostFilter(hosts ...string) HostFilter {
m := make(map[string]bool, len(hostInfos))
for _, host := range hostInfos {
- m[host.ConnectAddress().String()] = true
+ connAddr, _ := host.ConnectAddress()
+ m[connAddr.String()] = true
}
return HostFilterFunc(func(host *HostInfo) bool {
- return m[host.ConnectAddress().String()]
+ connAddr, _ := host.ConnectAddress()
+ return m[connAddr.String()]
Review Comment:
if `err != nil` it should just return `false`
##########
integration_test.go:
##########
@@ -139,7 +147,11 @@ func TestHostFilterInitial(t *testing.T) {
// we'll filter out the second host
filtered := clusterHosts[1]
cluster.HostFilter = HostFilterFunc(func(host *HostInfo) bool {
- if host.ConnectAddress().String() == filtered {
+ connAddr, err := host.ConnectAddress()
+ if err != nil {
+ t.Error(err)
Review Comment:
Maybe it makes more sense to use `t.Fatal` here? (and the 2 other similar
cases in this file)
##########
policies.go:
##########
@@ -970,7 +979,8 @@ func (d *rackAwareRR) AddHost(host *HostInfo) {
func (d *rackAwareRR) RemoveHost(host *HostInfo) {
dist := d.HostTier(host)
- d.hosts[dist].remove(host.ConnectAddress())
+ connAddr, _ := host.ConnectAddress()
+ d.hosts[dist].remove(connAddr)
Review Comment:
same as above, check in `AddHost` so we don't add hosts with invalid
addresses and then we can just return here when unable to get connAddr
##########
token.go:
##########
@@ -213,14 +213,15 @@ func (t *tokenRing) String() string {
buf.WriteString("){")
sep := ""
for i, th := range t.tokens {
+ connAddr, _ := th.host.ConnectAddress()
buf.WriteString(sep)
sep = ","
buf.WriteString("\n\t[")
buf.WriteString(strconv.Itoa(i))
buf.WriteString("]")
buf.WriteString(th.token.String())
buf.WriteString(":")
- buf.WriteString(th.host.ConnectAddress().String())
+ buf.WriteString(connAddr.String())
Review Comment:
Will this print an empty string if `err != nil` ? Printing empty string on
the address part could be fine but we could also print `nil` instead
##########
policies.go:
##########
@@ -735,7 +739,8 @@ func (r *hostPoolHostPolicy) SetHosts(hosts []*HostInfo) {
}
func (r *hostPoolHostPolicy) AddHost(host *HostInfo) {
- ip := host.ConnectAddress().String()
+ connAddr, _ := host.ConnectAddress()
+ ip := connAddr.String()
Review Comment:
if unable to get `connAddr` just return
##########
cassandra_test.go:
##########
@@ -3086,7 +3090,11 @@ func TestDiscoverViaProxy(t *testing.T) {
for _, host := range clusterHosts {
found := false
for _, hi := range session.pool.hostConnPools {
- if hi.host.ConnectAddress().String() == host {
+ connAddr, err := hi.host.ConnectAddress()
+ if err != nil {
+ t.Error(err)
Review Comment:
maybe this should be `t.Fatal` so the behavior of this test is consistent
after the change?
##########
session.go:
##########
@@ -847,11 +848,12 @@ func (qm *queryMetrics) hostMetrics(host *HostInfo)
*hostMetrics {
// hostMetricsLocked gets or creates host metrics for given host.
// It must be called only while holding qm.l lock.
func (qm *queryMetrics) hostMetricsLocked(host *HostInfo) *hostMetrics {
- metrics, exists := qm.m[host.ConnectAddress().String()]
+ connAddr, _ := host.ConnectAddress()
+ metrics, exists := qm.m[connAddr.String()]
Review Comment:
We definitely should not be creating metrics objects for hosts with invalid
connect addresses since they won't be used anyway.
##########
session.go:
##########
@@ -394,7 +394,8 @@ func (s *Session) reconnectDownedHosts(intv time.Duration) {
if gocqlDebug {
buf := bytes.NewBufferString("Session.ring:")
for _, h := range hosts {
- buf.WriteString("[" +
h.ConnectAddress().String() + ":" + h.State().String() + "]")
+ connAddr, _ := h.ConnectAddress()
Review Comment:
we shouldn't really ignore these errors, it's better to handle them than
assuming that `connAddr` is always valid, just `continue` if unable to get
`connAddr`
##########
control.go:
##########
@@ -423,16 +432,21 @@ func (c *controlConn) attemptReconnectToAnyOfHosts(hosts
[]*HostInfo) (*Conn, er
var conn *Conn
var err error
for _, host := range hosts {
+ connAddr, err := host.ConnectAddress()
+ if err != nil {
+ c.session.logger.Printf("gocql: %v\n", err)
Review Comment:
same as comment above
##########
policies.go:
##########
@@ -818,7 +825,8 @@ func (host selectedHostPoolHost) Info() *HostInfo {
}
func (host selectedHostPoolHost) Mark(err error) {
- ip := host.info.ConnectAddress().String()
+ connAddr, _ := host.info.ConnectAddress()
Review Comment:
if I'm reading the code correctly this should be fine if we ensure that
hosts with invalid connect addr are never added to `hostPoolHostPolicy`
##########
policies.go:
##########
@@ -723,7 +726,8 @@ func (r *hostPoolHostPolicy) SetHosts(hosts []*HostInfo) {
hostMap := make(map[string]*HostInfo, len(hosts))
for i, host := range hosts {
- ip := host.ConnectAddress().String()
+ connAddr, _ := host.ConnectAddress()
+ ip := connAddr.String()
Review Comment:
skip the host if unable to get `connAddr`
##########
policies.go:
##########
@@ -96,7 +96,8 @@ func (c *cowHostList) remove(ip net.IP) bool {
found := false
newL := make([]*HostInfo, 0, size)
for i := 0; i < len(l); i++ {
- if !l[i].ConnectAddress().Equal(ip) {
+ connAddr, _ := l[i].ConnectAddress()
+ if !connAddr.Equal(ip) {
Review Comment:
`add()` should check if `.ConnectAddress()` returns an error and if it does
it shouldn't add it. If we do that then yeah we can ignore the error here
because it's guaranteed to always be `nil`
##########
policies.go:
##########
@@ -519,7 +521,8 @@ func (t *tokenAwareHostPolicy) AddHosts(hosts []*HostInfo) {
func (t *tokenAwareHostPolicy) RemoveHost(host *HostInfo) {
t.mu.Lock()
- if t.hosts.remove(host.ConnectAddress()) {
+ connAddr, _ := host.ConnectAddress()
+ if t.hosts.remove(connAddr) {
Review Comment:
Same as above but needs to call `RemoveHost` on the fallback policy instead
of returning right away
##########
host_source.go:
##########
@@ -164,8 +164,9 @@ func (h *HostInfo) Equal(host *HostInfo) bool {
// prevent rlock reentry
return true
}
-
- return h.ConnectAddress().Equal(host.ConnectAddress())
+ hConnAddr, _ := h.ConnectAddress()
+ hostConnAddr, _ := host.ConnectAddress()
+ return hConnAddr.Equal(hostConnAddr)
Review Comment:
maybe something like this would be better?
```
hConnAddr, err1 := h.ConnectAddress()
hostConnAddr, err2 := host.ConnectAddress()
return err1 == err2 && hConnAddr.Equal(hostConnAddr)
```
##########
policies.go:
##########
@@ -756,7 +761,8 @@ func (r *hostPoolHostPolicy) AddHost(host *HostInfo) {
}
func (r *hostPoolHostPolicy) RemoveHost(host *HostInfo) {
- ip := host.ConnectAddress().String()
+ connAddr, _ := host.ConnectAddress()
+ ip := connAddr.String()
Review Comment:
same as above
##########
policies.go:
##########
@@ -862,10 +870,11 @@ func (d *dcAwareRR) AddHost(host *HostInfo) {
}
func (d *dcAwareRR) RemoveHost(host *HostInfo) {
+ connAddr, _ := host.ConnectAddress()
Review Comment:
just return if unable to get conn addr but also need to add a check for this
in `AddHost` so hosts with invalid connect addresses are not added
##########
policies.go:
##########
@@ -353,7 +354,8 @@ func (r *roundRobinHostPolicy) AddHost(host *HostInfo) {
}
func (r *roundRobinHostPolicy) RemoveHost(host *HostInfo) {
- r.hosts.remove(host.ConnectAddress())
+ connAddr, _ := host.ConnectAddress()
Review Comment:
We can't ignore the error here, we're basically trying to remove an address
that doesn't exist. Just return if there is an error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
