Add additional logging. Remove unnecessary router lookups. May addres… #1483
Conversation
| return err | ||
| } | ||
| if r == nil { | ||
| return errors.Errorf("no router with id [%v] found, closing connection", ch.Id()) |
There was a problem hiding this comment.
"closing connection" is not actually happening here and is misleading imo.
There was a problem hiding this comment.
changed to 'not accepting' so it's more accurate.
| if err := binding.Bind(newBindHandler(self.heartbeatOptions, r, self.network, self.xctrls)); err != nil { | ||
| return errors.Wrap(err, "error binding router") | ||
| r.Listeners = nil | ||
| if val, found := ch.Underlay().Headers()[int32(ctrl_pb.ContentType_ListenersHeader)]; found { |
There was a problem hiding this comment.
i assume you know that ctrl_pb.ContentType_ListenersHeader is always gonna be 4 bytes long so this is 'fine'.... just leaving that comment to point it out. if that's fine, cool...
There was a problem hiding this comment.
(i also see now that is how it always was, originally i thought tihs was new code so... nevermind prolly)
There was a problem hiding this comment.
ctrl_pb.ContentType_ListenersHeader is a ContentType which is an alias for an int32, so the cast is fine.
controller/handler_ctrl/accept.go
Outdated
| } | ||
| } | ||
| } else { | ||
| log.Info("no advertised listeners") |
There was a problem hiding this comment.
is this really an info type of message?
There was a problem hiding this comment.
It was Warn and I dropped it to Info. Dropped it further to Debug.
| } | ||
|
|
||
| log.Info("accepted new router connection") |
There was a problem hiding this comment.
why lose the existing, presumably useful, r.Id? is the [r/Id] not useful?
There was a problem hiding this comment.
There's already a logger field for routerId added at the top, so this was redundant.
plorenz
force-pushed
the
instant-sync-tidy
branch
from
08bc786 to
b270d18
Compare
…s #1460