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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Orchestrators refactoring and fork handling other orchestrators #70

Merged
merged 33 commits into from Aug 10, 2017

Conversation

4 participants
@tiagofilipe12
Member

tiagofilipe12 commented Aug 4, 2017

This pull request includes a patch on uid for spamming tasks with junction as discussed in #69. Though this is still incomplete the remaining code of the PR can be quite complex and so we should merge it with dev branch before fixing uids for good. In fact, fork helped a lot to understand how we could handle duplication of tasks inside a pipeline, though merkle tree like handling of uids might be better.

This PR uniforms the resulting keys from each orchestrator .info, e.g.:

joinInvocator.info = {
  tasks: tasks.map(task => task),  //returns an array of tasks
  type: 'join'  // used to check the type of orchestrator
}

Now, each orchestrator stores an array of tasks that are inside them, e.g., join(task1, task2) results in [task1, task2] in which each entry is the taskInvocator function or the orchestrator (join(task1, fork(task2, task3) results in [task1, fork(task2,task3)] where orchestrator is handled as tasks.

Also and more importantly fork suffered a major refactoring (to match the other two orchestrators 'shape' and to be able to handle properly junction and fork inside fork. To do so, fork inside fork needs to store outer fork downstream tasks, inside outer fork it has to store upstream tasks and downstream tasks for this inner fork and the tasks inside this fork as well. Similar procedures was adopted to handle junction. However, they differ in the way lineage is stored. While fork forces duplication of tasks by adding a new uid for each task inside inner fork and adds a new lineage for each task inside fork (see code here), junction just adds downstream and upstream tasks inside the fork, as well as the newResttasks (tasks after the fork) to the junction itself.

Also, in this PR a bunch of test cases were added to ./examples/pipelines/tests in order to test these changes in the orchestrators as well as the 'orchestration' between them.

I am still working to simplify the code and document it in GSoC17 repo, so don't merge it yet. Just check if you want to change anything or if you can test it (with my examples/pipelines/tests or other that you find suitable or challenging enough 馃槃. I also want to stress test it a little more.

tiagofilipe12 added some commits Aug 8, 2017

@tiagofilipe12

This comment has been minimized.

Member

tiagofilipe12 commented Aug 8, 2017

Progress on this PR is being documented here.
Meanwhile, merkle tree like uid generation was added to task creation in order to avoid duplication of tasks within the same pipeline (but still needs testing before merging).

@tiagofilipe12

This comment has been minimized.

Member

tiagofilipe12 commented Aug 9, 2017

Just refactored orchestrators function to have .uid as tasks. And re-started refactor on fork. Currently, fork just doesn't handle other forks, everything else seems to be properly working in current state.

lib/task.js Outdated
@@ -39,20 +42,23 @@ const task = (dispatch) => (props, operationCreator, instanceUid = null) => {
// First thing we need to do is create a `uid` for this task
// `hashes` is an object with input, output, params keys
// The `uid` is a hash of the `input`, `output`, and `params` hashes
const { uid, hashes } = generateUid(props)
let { uid, hashes } = generateUid(props)

This comment has been minimized.

@thejmazz

thejmazz Aug 9, 2017

Member

use const If possible, can use newUid in later part

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Aug 10, 2017

Member

This is cool and in fact it is more clear to distinguish between uid and newUid because they are added to different objects. newUid is used when task is in fact created and uid is used out of the scope of the pipeline so to the definition of the task itself before having context.

@@ -0,0 +1,11 @@
'use strict'
const orchestratorUidGenerator = (tasks) => {

This comment has been minimized.

@thejmazz

thejmazz Aug 9, 2017

Member

can use .map:

return tasks.map(task => task.info.uid)

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Aug 10, 2017

Member

This also works well

return junctionResults
}).asCallback(cb)
}
junctionInvocator.info = {
tasks: tasks.map(task => task), // returns an array of tasks

This comment has been minimized.

@thejmazz

thejmazz Aug 9, 2017

Member

can probably do tasks: tasks (unless immutability is important)

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Aug 10, 2017

Member

Also seems to work ok.

tiagofilipe12 added some commits Aug 10, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Aug 10, 2017

Codecov Report

Merging #70 into dev will decrease coverage by 0.29%.
The diff coverage is 73.17%.

Impacted file tree graph

@@           Coverage Diff            @@
##              dev     #70     +/-   ##
========================================
- Coverage   80.49%   80.2%   -0.3%     
========================================
  Files          36      37      +1     
  Lines         841     884     +43     
  Branches      101     104      +3     
========================================
+ Hits          677     709     +32     
- Misses        164     175     +11
Impacted Files Coverage 螖
lib/constants/default-task-state.js 100% <酶> (酶) 猬嗭笍
lib/reducers/collection.js 85.55% <0%> (酶) 猬嗭笍
lib/orchestrators/orchestrator-uid-generator.js 100% <100%> (酶)
lib/lifecycle/generate-uid.js 100% <100%> (酶) 猬嗭笍
lib/watermill.js 90.47% <100%> (酶) 猬嗭笍
lib/orchestrators/join.js 96.66% <100%> (+1.21%) 猬嗭笍
lib/orchestrators/fork.js 37.93% <28%> (-12.07%) 猬囷笍
lib/orchestrators/junction.js 89.28% <75%> (+0.39%) 猬嗭笍
lib/task.js 96.55% <91.66%> (-3.45%) 猬囷笍

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 20d56f3...d16531a. Read the comment docs.

@codecov-io

This comment has been minimized.

codecov-io commented Aug 10, 2017

Codecov Report

Merging #70 into dev will increase coverage by 1.38%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #70      +/-   ##
==========================================
+ Coverage   80.49%   81.88%   +1.38%     
==========================================
  Files          36       37       +1     
  Lines         841      861      +20     
  Branches      101      102       +1     
==========================================
+ Hits          677      705      +28     
+ Misses        164      156       -8
Impacted Files Coverage 螖
lib/constants/default-task-state.js 100% <酶> (酶) 猬嗭笍
lib/reducers/collection.js 85.55% <0%> (酶) 猬嗭笍
lib/task.js 100% <100%> (酶) 猬嗭笍
lib/orchestrators/orchestrator-uid-generator.js 100% <100%> (酶)
lib/lifecycle/generate-uid.js 100% <100%> (酶) 猬嗭笍
lib/orchestrators/join.js 96.49% <100%> (+1.03%) 猬嗭笍
lib/watermill.js 90.47% <100%> (酶) 猬嗭笍
lib/orchestrators/fork.js 100% <100%> (+50%) 猬嗭笍
lib/orchestrators/junction.js 89.28% <75%> (+0.39%) 猬嗭笍

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 20d56f3...8cdd6f5. Read the comment docs.

tiagofilipe12 added some commits Aug 10, 2017

return forkResults
}).asCallback(cb)
}
const forkInvocator = (cb = _.noop, ctx = defaultContext) => {}

This comment has been minimized.

@thejmazz

thejmazz Aug 10, 2017

Member

haha this diff

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Aug 10, 2017

Member

Yeah 馃槃

@tiagofilipe12 tiagofilipe12 merged commit 08903a5 into dev Aug 10, 2017

10 checks passed

codecov/patch 95% of diff hit (target 80.49%)
Details
codecov/project 81.88% (+1.38%) compared to 20d56f3
Details
continuous-integration/gitbook/epub GitBook build "epub" succeeded
Details
continuous-integration/gitbook/json GitBook build "json" succeeded
Details
continuous-integration/gitbook/mobi GitBook build "mobi" succeeded
Details
continuous-integration/gitbook/pdf GitBook build "pdf" succeeded
Details
continuous-integration/gitbook/website GitBook build "website" succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details

thejmazz added a commit that referenced this pull request Aug 10, 2017

@thejmazz thejmazz referenced this pull request Aug 10, 2017

Merged

Orchestrators tests #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment