task.dependencies #81
Conversation
routes/v1.js
Outdated
|
|
||
| // Ensure we have a self-dependency, this is how defineTask works now | ||
| if (!_.includes(taskDef.dependencies, taskId)) { | ||
| taskDef.dependencies.push(taskId); |
There was a problem hiding this comment.
@imbstack, so funny/scary story, this created a problem because our current validator just sets the default value from the json-schema without making a clone.
Hence, by doing dependencies.push(... I actually modified the default value inside the validator.
|
I'm reading this today, I swear. |
|
|
||
| // Try to create Task entity | ||
| try { | ||
| let runs = []; |
There was a problem hiding this comment.
A note for myself for later (this may be wrong): It looks like we always create tasks no matter whether or not they're ready to run. runs will just be empty until they're ready to actually run.
|
I'm about 3/4 way through reviewing this. Just thought that it might be a good idea to get the sentry-catches-crashes thing in here before this is shipped and rebase this on top of that change. It could save us a good bit of sadness down the road. What do you think? |
| @@ -1,5 +1,5 @@ | |||
| var base = require('taskcluster-base'); | |||
| var debug = require('debug')('queue:data'); | |||
| var debug = require('debug')('app'); | |||
|
Ok, I've read this. I want to spend some more time with it, but it seems pretty good. |
|
@imbstack, I think this is now done with tests and everything... |
|
@jhford you are also invited to have a look around :) |
| * } | ||
| */ | ||
| constructor(options = {}) { | ||
| assert(options, 'options are required'); |
| await this.dependencyTracker.resolveTask(m.taskId, m.resolution); | ||
| await m.remove(); | ||
| } catch (err) { | ||
| debug("[alert-operator] Failed to handle message: %j" + |
There was a problem hiding this comment.
I wish we had logging levels.
There was a problem hiding this comment.
Or reporting to sentry... Which seems like the better option here...
|
I think I'd still like to get the sentry exception handling stuff in here before this ships, but that's not necessary. r+ looks great to me. |
…o task-deps Conflicts: config.yml
| "scope-pattern": "^[\\x20-\\x7e]*$", | ||
|
|
||
| // Maximum number of dependencies a single task can have | ||
| "max-task-dependencies": 100, |
There was a problem hiding this comment.
It was a prettt round number :)
Obviously there is some limit, I don't know what it is... But 100 should serve most use-cases and force intermediate dummy tasks in the edge cases...
|
Did a once-over of this and it looks good. I'm a little curious about why we have the limit of dependent tasks at 100, but i get that we do want some sort of limit. |
3 participants
Work in progress adding
task.dependenciesandtask.dependencyRelation.At this stage I need to write tests... try to actually run the code and fix all the bugs that comes up :)
But it's also a decent time to start review for sanity...
I'm fairly confident I've designed it such that a crash anytime during
createTask, doesn't prevent the samecreateTaskcall from succeeding if retried later... It's hard to get the idempotency correct, but I think it's sane... please have a look.