joao-r-reis commented on code in PR #1895:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1895#discussion_r2159356941
##########
control.go:
##########
@@ -322,7 +322,11 @@ func (c *controlConn) setupConn(conn *Conn, sessionInit
bool) error {
iter := conn.querySystemLocal(context.TODO())
host, err := c.session.hostInfoFromIter(iter, conn.host.connectAddress,
conn.r.RemoteAddr().(*net.TCPAddr).Port)
if err != nil {
- return err
+ iter.Close()
Review Comment:
Should we handle the error returned by Close()?
##########
host_source.go:
##########
@@ -700,8 +747,12 @@ func (r *ringDescriber) getLocalHostInfo() (*HostInfo,
error) {
host, err := r.session.hostInfoFromIter(iter, nil, r.session.cfg.Port)
if err != nil {
+ iter.Close()
Review Comment:
Same as above, should we handle the error returned by this call?
##########
host_source.go:
##########
@@ -720,18 +770,25 @@ func (r *ringDescriber) getClusterPeerInfo(localHost
*HostInfo) ([]*HostInfo, er
return nil, errNoControl
}
- rows, err := iter.SliceMap()
- if err != nil {
- // TODO(zariel): make typed error
- return nil, fmt.Errorf("unable to fetch peer host info: %s",
err)
- }
-
- for _, row := range rows {
+ var peers []*HostInfo
+ for {
// extract all available info about the peer
- host, err := r.session.newHostInfoFromMap(nil,
r.session.cfg.Port, row)
+ host, err := r.session.hostInfoFromIter(iter, nil,
r.session.cfg.Port)
if err != nil {
- return nil, err
- } else if !isValidPeer(host) {
+ // if the error came from the iterator then return it,
otherwise ignore
+ // and warn
+ if iterErr := iter.Close(); iterErr != nil {
+ return nil, fmt.Errorf("unable to fetch peer
host info: %s", iterErr)
+ }
+ // skip over peers that we couldn't parse
+ r.session.logger.Warning("Failed to parse peer this
host will be ignored.", newLogFieldError("err", err))
Review Comment:
Yeah seems reasonable to me
##########
host_source.go:
##########
@@ -700,8 +747,12 @@ func (r *ringDescriber) getLocalHostInfo() (*HostInfo,
error) {
host, err := r.session.hostInfoFromIter(iter, nil, r.session.cfg.Port)
if err != nil {
+ iter.Close()
return nil, fmt.Errorf("could not retrieve local host info:
%w", err)
}
+ if host == nil {
+ return nil, errors.New("could not retrieve local host info:
query returned 0 rows")
+ }
Review Comment:
Same as my other comment, doesn't look like `host` can ever be `nil`
##########
host_source.go:
##########
@@ -413,8 +414,9 @@ func (h *HostInfo) update(from *HostInfo) {
if h.dataCenter == "" {
h.dataCenter = from.dataCenter
}
- if h.rack == "" {
+ if h.missingRack || h.rack == "" {
Review Comment:
Maybe we should remove the `h.rack == ""` from here? If now we treat `""` as
a valid rack.
##########
host_source.go:
##########
@@ -720,18 +770,25 @@ func (r *ringDescriber) getClusterPeerInfo(localHost
*HostInfo) ([]*HostInfo, er
return nil, errNoControl
}
- rows, err := iter.SliceMap()
- if err != nil {
- // TODO(zariel): make typed error
- return nil, fmt.Errorf("unable to fetch peer host info: %s",
err)
- }
-
- for _, row := range rows {
+ var peers []*HostInfo
+ for {
// extract all available info about the peer
- host, err := r.session.newHostInfoFromMap(nil,
r.session.cfg.Port, row)
+ host, err := r.session.hostInfoFromIter(iter, nil,
r.session.cfg.Port)
if err != nil {
- return nil, err
- } else if !isValidPeer(host) {
+ // if the error came from the iterator then return it,
otherwise ignore
+ // and warn
+ if iterErr := iter.Close(); iterErr != nil {
+ return nil, fmt.Errorf("unable to fetch peer
host info: %s", iterErr)
+ }
+ // skip over peers that we couldn't parse
+ r.session.logger.Warning("Failed to parse peer this
host will be ignored.", newLogFieldError("err", err))
+ continue
+ }
+ // if nil then none left
+ if host == nil {
+ break
Review Comment:
Is this ever a scenario? I'm reading the code and it doesn't look like it.
It doesn't look like `hostInfoFromMap` can ever return a nil host. It should
return either `nil, err` or `host, nil` where host is never nil.
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
