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.

Sign up for GitHub

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

Jump to bottom

Fixed scheduling ID of pipelines #109

Merged
merged 4 commits into from
Sep 25, 2018
Merged

Fixed scheduling ID of pipelines #109

merged 4 commits into from
Sep 25, 2018

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Sep 24, 2018 •
edited
Loading

Deals with #108

screen shot 2018-09-24 at 07 16 28

@codecov-io
Copy link

codecov-io commented Sep 24, 2018 •
edited
Loading

Codecov Report

Merging #109 into master will increase coverage by 0.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   64.33%   64.71%   +0.38%     
==========================================
  Files          24       22       -2     
  Lines        2044     1913     -131     
==========================================
- Hits         1315     1238      -77     
+ Misses        579      539      -40     
+ Partials      150      136      -14
Impacted Files Coverage Δ
scheduler/scheduler.go 74.73% <100%> (+2.19%) ⬆️
plugin/plugin.go
plugin/grpc.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 767ef36...2c82d8c. Read the comment docs.

@Skarlso Skarlso requested a review from michelvocks September 24, 2018 05:40
@michelvocks
Copy link
Member

Your fix does not work, @Skarlso 😞
This will increment the ID globally. Which means when you have multiple pipelines the IDs will be shared among all pipeline runs e.g. start pipeline A -> will get ID 1; start pipeline B -> will get ID 2 (should get also ID 1).

This is also incompatible to existing boltDB databases.

@Skarlso
Copy link
Member Author

Skarlso commented Sep 24, 2018

Oh buu. You're right. I should have tried with multiple pipelines.

Okay, I have another fix in mind. Thanks for the review.

Yeah, you are also right that this would require a redo of the DB. :/

@Skarlso Skarlso added Needs Work The PR still requires some work from the submitter. needs review and removed needs review Needs Work The PR still requires some work from the submitter. labels Sep 24, 2018
@Skarlso
Copy link
Member Author

Skarlso commented Sep 24, 2018

@michelvocks Alright. This time it's working with multiple pipelines and multiple runs with quick button smashes. It's also warding against if multiple users are using Gaia at the same time and are trying to trigger a build at the same time. :) I think you'll be happy with this one. Hopefully. :D

screen shot 2018-09-24 at 22 36 28

@Skarlso Skarlso force-pushed the pipeline_scheduling branch from 100336a to fbd475f Compare September 24, 2018 20:39
@Skarlso Skarlso force-pushed the pipeline_scheduling branch from fbd475f to 5c267a3 Compare September 25, 2018 04:24
Skarlso
Skarlso commented Sep 25, 2018
View reviewed changes
scheduler/scheduler.go Outdated
// This means that one of the calls will take slightly longer (a couple of nanoseconds)
// while the other finishes to save the pipelinerun.
// This is to ensure that the highest ID for the next pipeline is calculated properly.
if schedulerRunning {
Copy link
Member Author

Choose a reason for hiding this comment

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

why did I even put an if here... 🤦‍♂️

Skarlso
Skarlso commented Sep 25, 2018
View reviewed changes
scheduler/scheduler.go Outdated
// while the other finishes to save the pipelinerun.
// This is to ensure that the highest ID for the next pipeline is calculated properly.
if schedulerRunning {
for schedulerRunning {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I need to rethink this semaphore with a channel.

@Skarlso Skarlso added Needs Work The PR still requires some work from the submitter. and removed needs review labels Sep 25, 2018
@Skarlso
Copy link
Member Author

Skarlso commented Sep 25, 2018

Omg what am I doing. Just use a waitgroup lock unlock. That's what they are for.

@Skarlso Skarlso added Ready To Merge PR is ready to be merged into master and removed Needs Work The PR still requires some work from the submitter. labels Sep 25, 2018
michelvocks
michelvocks approved these changes Sep 25, 2018
View reviewed changes
Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

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

Awesome 🤗 ❤️

@michelvocks michelvocks merged commit 8e44588 into gaia-pipeline:master Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michelvocks michelvocks michelvocks approved these changes

Assignees
No one assigned
Labels
Ready To Merge PR is ready to be merged into master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Skarlso @codecov-io @michelvocks