-
Notifications
You must be signed in to change notification settings - Fork 3
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
impl: add async InsertJob #24
Conversation
Thanks for the LRO example! I learnt a lot from this. |
// Once we update the code in google-cloud-cpp to accept a operation_name | ||
// function and default it to OperationType::name, all this code can go away. |
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.
Did you do this already? googleapis/google-cloud-cpp#14924
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.
It's available at HEAD, but not in a google-cloud-cpp release this repo can depend on. Once google-cloud-cpp v2.34.0 is available, this can be refactored.
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.
Gotcha, thanks
// TODO: JobServiceRestConnectionImpl requires background threads, but does it | ||
// actually use them? Needs further investigation. |
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.
*RestConnectionImpl
requires background threads, but does it actually use them?
I don't think so.
I thought for REST, our only async APIs are the LROs. We spin off a new thread for each request to handle the polling asynchronously.
And then our long-term goal was to maybe have a background thread pool and do the work on that instead.
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.
Right, and I'm pretty sure that none of the services in bigquerycontrol have any LRO rpcs. Other REST services do have LROs, but in bigquerycontrol the background threads go unused.
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.
And then our long-term goal
FTR, it looks like we did implement this.
AsyncRestPollingLoopImpl
takes a CompletionQueue
which it gets from the background threads.
a lot of compute
uses this.
Adds the InsertJob overloads that starts the Job and returns a future while polling in the background for completion.
Created issue to add LRO unit tests #23