-
Notifications
You must be signed in to change notification settings - Fork 126
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
Only call GetJobSetEvents once #2442
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2442 +/- ##
=======================================
Coverage 58.48% 58.48%
=======================================
Files 234 234
Lines 29239 29241 +2
=======================================
+ Hits 17099 17103 +4
+ Misses 10827 10826 -1
+ Partials 1313 1312 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Just a quick question about logging in one place, otherwise I think it looks good.
@@ -82,6 +82,17 @@ func (eventToJobService *EventsToJobService) streamCommon(ctx context.Context, t | |||
} | |||
}() | |||
|
|||
log.Infof("GetJobEventMessage for %s/%s with id %s", eventToJobService.queue, eventToJobService.jobSetId, fromMessageId) |
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.
Are Daniele and/or others comfortable with info-level logging on this, or would they prefer it to be debug-level?
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 it would be fine as this should only be once every subscribe. But we can always mess around with 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.
LGTM, though I echo Rich's comment on possibly lowering the log-level for every message to debug.
When using a GRPC stream you should get the stream and then start looping over that. Calling it a bunch is not ideal.
This is a POC for right now to test out if this actually works.
Thanks @d80tb7
┆Issue is synchronized with this Jira Task by Unito