-
Notifications
You must be signed in to change notification settings - Fork 82
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
Pass name of connection to logs #843
Conversation
select { | ||
case ci.WriteChan <- message: | ||
case <-ci.Context.Done(): | ||
s.Logger.Debug("connInfo cancelled during flood write") | ||
s.Logger.Debug("connInfo for connection %s cancelled during flood write", conn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would s.NodeID()
be helpful in this log also @fosterseth ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not necessary in this case
Overall the PR is looking good, Please have a look at the guideline we are following here; https://cbea.ms/git-commit/ Capitalize the subject line |
5f65ebc
to
7291f57
Compare
Codecov Report
@@ Coverage Diff @@
## devel #843 +/- ##
==========================================
+ Coverage 30.28% 30.29% +0.01%
==========================================
Files 44 44
Lines 8530 8530
==========================================
+ Hits 2583 2584 +1
+ Misses 5708 5707 -1
Partials 239 239
|
pkg/netceptor/netceptor.go
Outdated
s.Logger.Warning("Timing out connection, idle for the past %s\n", s.maxConnectionIdleTime) | ||
timedOut[i]() | ||
for conn := range timedOut { | ||
s.Logger.Warning("Timing out connection from %s to %s, idle for the past %s\n", s.nodeID, conn, s.maxConnectionIdleTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the from nodeA to nodeB
implies connection direction. Meaning I interpret that as nodeA is dialing a tcp listener on nodeB. However, it could be that nodeB dialed nodeA.
I think we should drop s.nodeID
and just say "timing out connection %s, idle for the past %s`
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes issue #840 by passing the connection name for both timeout connection log in monitorConnectionAging method and connection cancelled log in flood method.