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

feat(chord): ErrorCallback to handle Group failures #605

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ggodik
Copy link

@ggodik ggodik commented Oct 9, 2020

A chord is a Group and a task that executes when the Group is done with the results of the group.
If a task fails in that group - nothing gets called at the end of the group execution.

Proposal: NewChordWithError which would make a Chord(Group, SuccessTask, FailureTask) using the following logic:

if all tasks in group succeed:
     SuccessTask(results of group)
else:
     FailureTask(errors of failed tasks)

The FailureTask would take an aggregate of errors of all the failed tasks e.g. func FailureTask(errs []string) error
Any successful completions of the tasks in the group would be 'dropped'

Changes in this PR

  • tasks.NewChordWithError() function that takes an additional Signature
  • tasks.NewChord maintains backwards compatiblity
  • results.AsyncChordResult behavior modified to fetch ErrorCallback results if one of the tasks fails
  • worker.go modifications for processing ErrorChord
  • Consolidated Chord Callback logic handling in single function: processChord
  • Interfaces and Mocks for Server and Worker
  • Unit tests
  • Integration tests

Note: Unit tests made this a larger change as they required making interfaces out of Server and Worker and injecting those into processing functions. I'd love to take this effort further - but I wanted to keep this change as small as possible. It's already 16 files so I somewhat failed at that.

Let me know if you have thoughts on the utility of the change, etc

@ggodik
Copy link
Author

ggodik commented Oct 9, 2020

cc @zeroshade

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #605 (c0f13fa) into master (831c54f) will decrease coverage by 1.80%.
The diff coverage is 72.50%.

❗ Current head c0f13fa differs from pull request most recent head 26f15fe. Consider uploading reports for the commit 26f15fe to get more accurate results

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   40.86%   39.05%   -1.81%     
==========================================
  Files          34       32       -2     
  Lines        2763     2929     +166     
==========================================
+ Hits         1129     1144      +15     
- Misses       1490     1641     +151     
  Partials      144      144              
Impacted Files Coverage Δ
v1/server.go 27.55% <0.00%> (+5.03%) ⬆️
v1/tasks/signature.go 46.15% <ø> (+14.57%) ⬆️
v2/server.go 25.42% <0.00%> (ø)
v1/worker.go 16.58% <70.96%> (+14.35%) ⬆️
v1/tasks/workflow.go 90.32% <100.00%> (+63.39%) ⬆️
v1/common/broker.go 73.33% <0.00%> (-5.05%) ⬇️
v1/common/redis.go 0.00% <0.00%> (ø)
v1/config/config.go 75.00% <0.00%> (ø)
v1/backends/redis/redis.go 67.08% <0.00%> (ø)
v1/backends/redis/goredis.go 0.00% <0.00%> (ø)
... and 11 more

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 7ccba3d...26f15fe. Read the comment docs.

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.

None yet

2 participants