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

Target param ignored during retry attempts and barrier notifications. #60

Closed
someone1 opened this issue Nov 9, 2015 · 4 comments
Closed

Target param ignored during retry attempts and barrier notifications. #60

someone1 opened this issue Nov 9, 2015 · 4 comments

Comments

@someone1
Copy link
Contributor

someone1 commented Nov 9, 2015

Commit 443e311 addressed this previously, but it was reversed. I'll put in a PR shortly that tries it a different way.
someone1 added a commit to someone1/appengine-pipelines that referenced this issue Nov 9, 2015
@someone1
Fixes Issue GoogleCloudPlatform#60
cd692c3 
This was tried differently before in commit 443e311.

For barrier notifications, this was tried before by getting all barrier records' pipeline keys and doing a batch get, but that method did not account for situations in which barrier checks were fired but no pipes were ready yet. My method will potentially result in more RPC requests, but only pipes that are ready to be executed will be fetched. 

For retries: we already have the `pipeline_record` fetched, just pass in the target parameter
@sit
Copy link

sit commented Jan 25, 2016

Can someone test/merge this?

@AngryBrock @tkaitchuck

@tkaitchuck
Copy link
Contributor

This looks like it is fixed here:
#61
@AngryBrock Any objections to merging?

On Sun, Jan 24, 2016 at 10:23 PM, Emil Sit notifications@github.com wrote:

Can someone test/merge this?

@AngryBrock https://github.com/AngryBrock @tkaitchuck
https://github.com/tkaitchuck

—
Reply to this email directly or view it on GitHub
#60 (comment)
.

@AngryBrock
Copy link
Contributor

@tkaitchuck no objections from me.

tkaitchuck added a commit that referenced this issue Jan 27, 2016
@tkaitchuck
Merge pull request #61 from someone1/patch-4
5b4cff2 
Fixes Issue #60
@tkaitchuck
Copy link
Contributor

Merged

@sit sit mentioned this issue Jan 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sit @AngryBrock @someone1 @tkaitchuck