-
Notifications
You must be signed in to change notification settings - Fork 448
Add jobs.wait_for_job method
#903
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
Conversation
|
It makes sense to me. The back of my head is asking if the Maybe that's too much, it's a bit off-the-cuff. Let me know what you think, otherwise I'll give this code as is a deeper review |
|
Looks good to me. I don't really agree with @t8y8 about a generalizable TSC.Waiter class - I think all async operations spawn a background job and so this is the right place to implement this wait logic. |
But I see what you mean here... To me the main question would be:
which makes me wonder: which other things would there in the Tableau Server API which we might want to wait on? But afaict the "jobs" concept is already Tableau's abstraction for everything async on the server, and hence introducing another layer of abstraction on the client would add only limited value |
t8y8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes.
I also want to verify this works for AD sync, and I think it does (
| def create_AD_group(self, group_item, asJob=False): |
|
The other thing we can wait for is a Flow Run, which might actually be what the BackgroundJobItem is related to now I look some more. @jorwoods how does this look to you in the context of adding the flows endpoints? |
|
Regarding any changes to #884, this would not be too difficult to implement on the |
|
argh... Ok, so flows are also de-facto jobs, but for some reason didn't use the jobs API. That's sad 😢 Such a nice abstraction on the server side, but then its inconistent by one job type not using the common REST interface... @jacalata maybe the "jobs" endpoint/abstraction should be added as a guideline to the TAG design guidelines for our REST API? Either way, it is what it is... I will rewrite this code to pull out the "retry with exponential backoff" logic, such that it can be reused across multiple places, in particular also by @jorwoods in the flow endpoint, without having to duplicate that logic. |
a44c63d to
4eee552
Compare
|
added test cases and refactored the waiting logic into a reusable I think this should be ready for the final review now |
4eee552 to
5252170
Compare
This commit adds a `wait_for_job` method which will repeatedly poll a job's status until that job is finished. Internally, it uses an exponential backoff for the polling intervals. That way, it is snappy for fast-running jobs without putting too much load on the server for long-running jobs. It returns the successfully finished `JobItem` object which might be of interest to the caller, e.g. to inspect the reported `started_at` `finished_at` times or the `notes`. For failed jobs, `wait_for_job` raises an exception. That way, we ensure that errors in jobs don't accidentally go unnoticed. The `jobs` object can still be retrieved from the exception object, if required.
5252170 to
2e8dc1e
Compare
t8y8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great -- thanks for doing the diligence and discovering flows don't fit the pattern. (I'm not too surprised they don't, there was significant time between BG Jobs and Flows Runs).
The testing of the BackoffTimer is excellent -- I'm excited that a bonus of this implementation is that anyone can use the timer to write their own backoff between retries of non job calls, like waiting for SOLR to update or something.
🚀
This commit adds the corresponding documentation for PR tableau#903
Should have been part of #903, but I forgot to `git add` them :/
Based on @FrancoisZim 's suggestion.
Currently still a draft to get feedback on the proposed interface. Will add test cases, as soon as we got agreement on the interface & functionality.
This commit adds a
wait_for_jobmethod which will repeatedly polla job's status until that job is finished.
Internally, it uses an exponential backoff for the polling intervals.
That way, it is snappy for fast-running jobs without putting too
much load on the server for long-running jobs.
It returns the successfully finished
JobItemobject which might beof interest to the caller, e.g. to inspect the reported
started_atfinished_attimes or thenotes.For failed jobs,
wait_for_jobraises an exception. That way, weensure that errors in jobs don't accidentally go unnoticed. The
jobsobject can still be retrieved from the exception object, ifrequired.