DependsOn job dependency management #67

michelvocks · 2018-08-01T20:09:53Z

Fixes #19 .

codecov-io · 2018-08-06T13:27:26Z

Codecov Report

Merging #67 into master will increase coverage by 2.9%.
The diff coverage is 87.66%.

@@            Coverage Diff            @@
##           master      #67     +/-   ##
=========================================
+ Coverage   60.98%   63.88%   +2.9%     
=========================================
  Files          18       19      +1     
  Lines        1548     1653    +105     
=========================================
+ Hits          944     1056    +112     
+ Misses        500      489     -11     
- Partials      104      108      +4

Impacted Files	Coverage Δ
scheduler/workload.go	`100% <100%> (ø)`
scheduler/scheduler.go	`75.09% <83.76%> (+12.93%)`	⬆️

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 8b39c4e...ae46472. Read the comment docs.

michelvocks · 2018-08-06T17:41:05Z

@Skarlso huge PR. I'm sorry 😥

A few points related to this PR:

It currently points to my proto-fork. See the MR here: DependsOn implemented. Changed from Question to ManualInteraction. protobuf#1
DependsOn implementation gosdk#2 for the gosdk.
I updated all dependencies via dep ensure -update. Not sure if this was a good idea 😈
Currently, you have to write the Title of a job into the depends-on list (which is a []string). I'm not a huge fan of this due to the fact that it's easy to introduce a typo. I was thinking about linking the jobs directly e.g.:

job1 := sdk.Job{
    Title: "myjob",
    Handler: MyHandler,
}
job2 := sdk.Job{
    Title: "otherjob",
    Handler: OtherHandler,
    DependsOn: []sdk.Job{job1},
}

That would work but if your have a few jobs it will be a mess to handle. Especially when you want to change the depends-on structure. You end up coping all jobs in a specific order so that they exist... which sucks.

Skarlso · 2018-08-08T19:53:27Z

Gah!! Somehow I didn't get the email on the ping. :( So sorry @michelvocks. You can ping me in slack as well, if there is a PR to check. I'll get that for sure. :) Looking over it now.

Skarlso

Some nits for now. I will need to take more rounds with this. It is indeed quiet large. :/

Skarlso · 2018-08-09T19:46:15Z

scheduler/scheduler.go

+		}
+
+		// Job has been resolved
+		if inResolved {


A nicer and better way of doing this is actually using labels.

OUTER: for _, job := range j.DependsOn { ... var inResolved bool for _, resolvedJob := range resolved { if resolvedJob.ID == job.ID { continue OUTER } } ... // continue doing stuff here

Skarlso · 2018-08-09T19:47:54Z

scheduler/scheduler.go

+
+		// Job has been resolved
+		if resolved {
+			continue


Same as above. Use labels. Oh you might not be able to do that here since Iter is channels...

Yeah right. I actually hate this iter-channel stuff. You cannot leave the loop otherwise you get a deadlock. Might need to change that in the future.

There is no real benefit unless it's called multiple times from different routines.

Skarlso · 2018-08-09T19:49:58Z

scheduler/scheduler.go

+	// quit function if signaled.
+	// We do not block here because this is just a pre-validation step.
+	select {
+	case <-quit:


As per convention this is rather called done.

Skarlso · 2018-08-09T19:50:25Z

scheduler/scheduler.go

+// After a workload has been send to the executenSCheduler, the method will
+// block and wait until the workload is done.
+// This method is designed to be called recursive and blocking.
+func (s *Scheduler) resolveDependencies(j gaia.Job, mw *managedWorkloads, executenScheduler chan gaia.Job, quit chan bool) {


executenScheduler German a bit? :)

Oh, it was late. 😄

Skarlso · 2018-08-09T19:59:33Z

scheduler/workload.go

+	defer mw.Unlock()
+
+	// Search for the id
+	var i = -1


just i := -1

Skarlso · 2018-08-10T04:20:09Z

frontend/client/views/pipeline/detail.vue

+        // Iterate all dependent jobs
+        for (let x = 0, y = deps.length; x < y; x++) {
+          // iterate again all jobs
+          for (let a = 0, b = jobs.length; a < b; a++) {


you got to have a better name for these buddy. :) It's really getting hard to follow all these as and xs and ys.

will rename them

Skarlso · 2018-08-10T04:20:51Z

plugin/plugin.go

 	plugin "github.com/hashicorp/go-plugin"
+	"github.com/michelvocks/protobuf"


Oh, I just noticed that this is also now your protobuf. I think this might be an auto import leftover?

It's currently my fork otherwise it would not compile. I have to change it for sure before we merge this 😄

michelvocks · 2018-08-10T06:49:59Z

@Skarlso fixed your suggestions. Thanks a lot 🤗

bidhan-a · 2018-08-10T16:32:18Z

scheduler/workload.go

+	return &foundWorkload
+}
+
+// Replace takes the given worklaod and replaces it in the managedWorkloads


Typo: worklaod => workload

bidhan-a · 2018-08-10T16:42:45Z

scheduler/scheduler.go

+// This method is designed to be called recursive and blocking.
+func (s *Scheduler) resolveDependencies(j gaia.Job, mw *managedWorkloads, executeScheduler chan gaia.Job, done chan bool) {
+	for _, depJob := range j.DependsOn {
+		// Check if this workdload is already resolved


workdload => workload

updated

Skarlso · 2018-08-11T07:39:27Z

@michelvocks I added some tests for workloads as well. Going to add one more after that it's ready to merge. :)

Skarlso · 2018-08-11T08:16:05Z

@michelvocks I think this is ready to merge. :) 100% coverage for workload. :)

michelvocks self-assigned this Aug 1, 2018

michelvocks force-pushed the dependson branch 2 times, most recently from 6faa4ed to 332af72 Compare August 6, 2018 10:17

michelvocks force-pushed the dependson branch 2 times, most recently from 041da49 to abd2434 Compare August 6, 2018 17:27

michelvocks changed the title ~~[WIP][POC] DependsOn job dependency management~~ [WIP] DependsOn job dependency management Aug 6, 2018

michelvocks requested a review from Skarlso August 6, 2018 17:27

michelvocks added the Needs Work The PR still requires some work from the submitter. label Aug 6, 2018

DependsOn implemented

79ced2d

michelvocks force-pushed the dependson branch from abd2434 to 79ced2d Compare August 7, 2018 06:39

michelvocks changed the title ~~[WIP] DependsOn job dependency management~~ DependsOn job dependency management Aug 8, 2018

michelvocks removed the Needs Work The PR still requires some work from the submitter. label Aug 8, 2018

Skarlso previously requested changes Aug 9, 2018

View reviewed changes

Skarlso reviewed Aug 10, 2018

View reviewed changes

Fixed comments/review points

fce4965

bidhan-a reviewed Aug 10, 2018

View reviewed changes

michelvocks mentioned this pull request Aug 10, 2018

Pipeline parameters #71

Merged

Added tests for workload and a slight fix of select.

09f39b0

Added another test. Done.

10e0088

Skarlso added the Ready To Merge PR is ready to be merged into master label Aug 11, 2018

100% coverage for workload.

963c0c5

Fixed typo.

ae46472

michelvocks force-pushed the dependson branch from a1a5442 to ae46472 Compare August 12, 2018 09:19

michelvocks merged commit 22073ca into gaia-pipeline:master Aug 12, 2018

michelvocks deleted the dependson branch August 12, 2018 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DependsOn job dependency management #67

DependsOn job dependency management #67

michelvocks commented Aug 1, 2018

codecov-io commented Aug 6, 2018 •

edited

Loading

michelvocks commented Aug 6, 2018 •

edited

Loading

Skarlso commented Aug 8, 2018

Skarlso left a comment

Skarlso Aug 9, 2018

Skarlso Aug 9, 2018

michelvocks Aug 10, 2018

Skarlso Aug 10, 2018

Skarlso Aug 9, 2018

Skarlso Aug 9, 2018

michelvocks Aug 10, 2018

Skarlso Aug 9, 2018

Skarlso Aug 10, 2018

michelvocks Aug 10, 2018

Skarlso Aug 10, 2018

michelvocks Aug 10, 2018

michelvocks commented Aug 10, 2018

bidhan-a Aug 10, 2018

bidhan-a Aug 10, 2018

Skarlso commented Aug 11, 2018

Skarlso commented Aug 11, 2018 •

edited

Loading

		plugin "github.com/hashicorp/go-plugin"
		"github.com/michelvocks/protobuf"

DependsOn job dependency management #67

DependsOn job dependency management #67

Conversation

michelvocks commented Aug 1, 2018

codecov-io commented Aug 6, 2018 • edited Loading

Codecov Report

michelvocks commented Aug 6, 2018 • edited Loading

Skarlso commented Aug 8, 2018

Skarlso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelvocks commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso commented Aug 11, 2018

Skarlso commented Aug 11, 2018 • edited Loading

codecov-io commented Aug 6, 2018 •

edited

Loading

michelvocks commented Aug 6, 2018 •

edited

Loading

Skarlso commented Aug 11, 2018 •

edited

Loading