Skip to content
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

Implement basic queueing #474

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Implement basic queueing #474

merged 3 commits into from
Mar 30, 2022

Conversation

michaelsauter
Copy link
Member

@michaelsauter michaelsauter commented Mar 21, 2022

Pipeline runs belonging to one repository now run sequentially. If a
pipeline run cannot start immediately, it is created as "pending", and a
process is started to periodically check if it can start.

Since the pipeline manager service may be restarted, it check on boot if
there are any pending runs for the repositories under its control and
starts the periodic check for those.

We can improve on the design by adding a signal in the finish task of a
pipeline run that the run will soon finish, reducing the up to 30s wait
time for the next run.

Closes #394.

Tasks:

  • Updated design documents in docs/design directory or not applicable
  • Updated user-facing documentation in docs directory or not applicable
  • Ran tests (e.g. make test) or not applicable
  • Updated changelog or not applicable

@michaelsauter michaelsauter self-assigned this Mar 21, 2022
@michaelsauter michaelsauter force-pushed the feature/queueing-new branch 2 times, most recently from aeed38c to dd3a260 Compare March 21, 2022 10:12
@michaelsauter michaelsauter mentioned this pull request Mar 23, 2022
4 tasks
@michaelsauter
Copy link
Member Author

Please do not review this now. I would like to merge #480 first, then rebase this PR so that it becomes way smaller and easier to digest.

@michaelsauter
Copy link
Member Author

michaelsauter commented Mar 24, 2022

@kuebler @henninggross @gerardcl ready for a first review pass. I'm not really happy with it but also unsure what exactly to improve. Let me know if you have ideas!

@michaelsauter michaelsauter force-pushed the feature/queueing-new branch 11 times, most recently from 9761aca to b675529 Compare March 28, 2022 12:40
@michaelsauter michaelsauter marked this pull request as ready for review March 28, 2022 12:40
internal/manager/queue.go Outdated Show resolved Hide resolved
internal/manager/queue.go Outdated Show resolved Hide resolved
Pipeline runs belonging to one repository now run sequentially. If a
pipeline run cannot start immediately, it is created as "pending", and a
process is started to periodically check if it can start.

Since the pipeline manager service may be restarted, it check on boot if
there are any pending runs for the repositories under its control and
starts the periodic check for those.

We can improve on the design by adding a signal in the finish task of a
pipeline run that the run will soon finish, reducing the up to 30s wait
time for the next run.

Closes #394.
Copy link
Collaborator

@kuebler kuebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

Copy link
Member

@henninggross henninggross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all it LGTM.
I am a fan of explicit variable names and would probably have called all variables concerning pipeline runs pipelineRun instead of pr (e.g. oldestPipelineRun instead of oldestPr), especially because outside of ods-pipeline the abbreviation PR in most cases means pull request, but that is personal preference and does not need to be implemented.

internal/manager/server.go Outdated Show resolved Hide resolved
@michaelsauter
Copy link
Member Author

@henninggross re: variable name - in Go, short names aren't seen as bad practice per se, rather it depends on the context. I kind of try to follow https://github.com/golang/go/wiki/CodeReviewComments#variable-names.

@michaelsauter michaelsauter merged commit 129b6cd into master Mar 30, 2022
@michaelsauter michaelsauter deleted the feature/queueing-new branch March 30, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race conditions when having more than one pipeline of the same branch
3 participants