-
Notifications
You must be signed in to change notification settings - Fork 14
Log completion message flow #236
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
+ Coverage 67.70% 67.77% +0.06%
==========================================
Files 24 24
Lines 3044 3050 +6
==========================================
+ Hits 2061 2067 +6
Misses 627 627
Partials 356 356
Continue to review full report at Codecov.
|
| msg, err := m.completeMessage(chid) | ||
| if err != nil { | ||
| return nil | ||
| return err |
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.
Is this changed on purpose? I see everything else is log lines, apart from this.
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.
This needs an explanation, as it introduces a change in behaviour.
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.
Hey,
The error returned from here is only logged and we take no action based on it. If there's an error while creating the complete message, we do want to know about it.
nonsense
left a comment
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, just one comment :)
| msg, err := m.completeMessage(chid) | ||
| if err != nil { | ||
| return nil | ||
| return err |
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.
This needs an explanation, as it introduces a change in behaviour.
@raulk This is to debug the "Timed out waiting for complete message" issue.