-
Notifications
You must be signed in to change notification settings - Fork 59
feat: adding job streaming support #812
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
base: main
Are you sure you want to change the base?
feat: adding job streaming support #812
Conversation
9f1c627
to
3afef6f
Compare
thanks for the PRs Lennart! i will take a look at these next week! |
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.
cloned and tested locally, everything works as advertised! great work!
var jobCount = maxJobsActive - currentJobs; | ||
activateJobsRequest.MaxJobsToActivate = jobCount; |
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 this is to set the limit for the activation request we use during polling, right? I think the calculation is wrong here? Because we missed updating the currentJobs as we just received a job.
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.
Actually looking at the code I think it is not necessary to change the request here or calculate this. I think we can remove this
var jobCount = maxJobsActive - currentJobs; | ||
activateJobsRequest.MaxJobsToActivate = jobCount; | ||
|
||
logger?.LogInformation( |
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.
maybe debug is better?
if (!await stream.ResponseStream.MoveNext(cancellationToken)) | ||
{ | ||
logger?.LogDebug("Job stream MoveNext returned false; retrying shortly"); | ||
await Task.Delay(50, cancellationToken); |
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.
Use a constant value... there are better options)
These delays may not be not correct for hiload.
In my code (local fork) I'v moved these values to the app settings. In this line I use 10ms, and on line 156 I use 100ms.
closes #661
inspired by @dimasm61
- Opens a long‑lived stream; jobs are pushed without polling.
- Backfilling threshold at 60% of MaxJobsActive to avoid over‑buffering.