Skip to content
This repository has been archived by the owner on Nov 11, 2022. It is now read-only.

Check for existence of job before polling. Closes #330. #331

Closed

Conversation

ravwojdyla
Copy link
Contributor

In case of local runners, export job does not exists, check if job
exists before polling for result. This will prevent unnecessary
exceptions, polling in local mode.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@ravwojdyla ravwojdyla force-pushed the 330_job_exists branch 2 times, most recently from bd7556f to 2f4b54d Compare July 6, 2016 03:47
@ravwojdyla
Copy link
Contributor Author

@peihe any comment on this one?

@dhalperi
Copy link
Contributor

Pei: please take a look

*
* <p>Returns null if the {@code maxAttempts} retries reached.
*/
Boolean exists(JobReference jobRef, int maxAttempts) throws InterruptedException;
Copy link
Contributor

@peihe peihe Jul 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggests to define the new function as:
/**
Gets the Job by {@link JobReference}.

Returns null if the job is not found. */ Job getJob(JobReference jobRef) throws InterruptedException, IOException;

The reason is: 1. getJob doesn't need to wait for the job to finish. 2. getJob is more general than exists.
Use the default BackOff for IOException:
new AttemptBoundedExponentialBackOff(MAX_RPC_ATTEMPTS, INITIAL_RPC_BACKOFF_MILLIS)

And, In the CleanupOperation, you can replace the pollJob() with getJob().

…#330.

In case of local runners, export job does not exists, check if job
exists before polling for result. This will prevent unnecessary
exceptions, polling in local mode.
@ravwojdyla
Copy link
Contributor Author

@peihe applied your suggestions.

bjchambers pushed a commit to bjchambers/DataflowJavaSDK that referenced this pull request Jul 26, 2016
@ravwojdyla
Copy link
Contributor Author

ping @peihe

@peihe
Copy link
Contributor

peihe commented Aug 2, 2016

sorry for the delay. This PR looks good to me.
And, I appended some additional fixups in #351

Thanks for submitting the fix!

@peihe
Copy link
Contributor

peihe commented Aug 17, 2016

This RP is merged in #351

@peihe peihe closed this Aug 17, 2016
@ravwojdyla
Copy link
Contributor Author

@peihe for some reason I can't find my commits in the history? Do you know what happen?

@ravwojdyla ravwojdyla deleted the 330_job_exists branch September 14, 2016 21:21
@ravwojdyla
Copy link
Contributor Author

Ping @peihe

@dhalperi
Copy link
Contributor

Hi Rav,

It looks like I messed up. GitHub automatically squashes pull requests into a single commit when merging, unless you specifically opt out. When I merged #351, which Pei sent as 3 commits, they got squashed fully into 1 commit.

On the command line, rebase+squash like this will result in git assigning authorship to the author of this first commit -- that should have been you. This is what we normally do when merging an external contributor's submission with minor fixups. However, it looks like GitHub may instead attribute authorship to the GitHub user who opens the PR, which in this case was @peihe.

Sorry for the gaffe. This difference in functionality is something I will watch out for this in the future.

At this point, we cannot retroactively repair the history. However, if you like I will revert (at HEAD) the squash and try to manually merge #351 again to properly attribute credit to you.

@ravwojdyla
Copy link
Contributor Author

ravwojdyla commented Oct 3, 2016

@dhalperi I see. No need for manual fix. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants