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

Long Running Operation APIs don't have a way to determine when the operation is created #7658

Closed
dpcollins-google opened this issue Nov 23, 2021 · 11 comments
Assignees
Labels
cpp: backlog While desirable, we do not have time to work on this for the foreseeable future. cpp: generator Issues related to the C++ micro-generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@dpcollins-google
Copy link
Contributor

Does this issue affect the google-cloud-cpp project?
Yes

What component of google-cloud-cpp is this related to?
Any that use google.longrunning.Operation in their APIs.

Describe the bug
The purpose of long running operations is that they are long running and have two phases: submission, and completion. There is no way with the current api, which returns a future with the response_type, to determine when submission has succeeded. This means it is impossible to write a short running command line tool that would create the operation and print its identifier for later use without blocking on the result.

@dpcollins-google dpcollins-google added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Nov 23, 2021
@dpcollins-google
Copy link
Contributor Author

The specific issue for me is with pubsublite, where the operation for SeekSubscription will never complete until the user connects a subscriber client for the first time or seeks again. This means it is impossible to write an infrastructure tool with this API that creates a subscription seeked to a predetermined location.

@coryan coryan added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. cpp: generator Issues related to the C++ micro-generator and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Nov 23, 2021
@coryan
Copy link
Contributor

coryan commented Nov 23, 2021

It should be possible to split the LRO into a *Client::SubmitFoo() function, returning a future<StatusOr<Operation>> and a AwaitFoo() overload set, returning future<StatusOr<ReturnType>> and consuming either StatusOr<Operation> or future<StatusOr<Operation>>.

@coryan
Copy link
Contributor

coryan commented Apr 8, 2022

Some ideas on this. Where today we have something like:

  future<StatusOr<google::spanner::admin::database::v1::Backup>> CreateBackup(
      google::spanner::admin::database::v1::CreateBackupRequest const& request,
      Options opts = {});

In the future we would have two new functions:

  future<StatusOr<google::longrunning::Operation>> StartCreateBackup(
      google::spanner::admin::database::v1::CreateBackupRequest const& request,
      Options opts = {});
  future<StatusOr<google::spanner::admin::database::v1::Backup>> AwaitCreateBackup(
      google::longrunning::Operation op,
      Options opts = {});

And the old function would be implemented using:

  future<StatusOr<google::spanner::admin::database::v1::Backup>> CreateBackup(
      google::spanner::admin::database::v1::CreateBackupRequest const& request,
      Options opts = {}) {
    return StartCreateBackup(request, opts).then([options = std::move(opts)](auto f) mutable {
      auto op = f.get();
      if (op) return AwaitCreateBackup(*std::move(op), std::move(options));
      return make_ready_future(StatusOr<google::spanner::admin::database::v1::Backup>>(std::move(op).status()));
    });
  }

@devbww
Copy link
Contributor

devbww commented Apr 8, 2022

Presumably that StartCreateBackup() returns something more like future<Operation> than future<Backup>.

@coryan
Copy link
Contributor

coryan commented Apr 8, 2022

Presumably that StartCreateBackup() returns something more like future<Operation> than future<Backup>.

Doh! Fixed.

@devbww
Copy link
Contributor

devbww commented Apr 8, 2022

I think that still begs the question of how CreateBackup() should behave when AwaitCreateBackup() exhausts its polling policy.

@coryan
Copy link
Contributor

coryan commented Apr 8, 2022

I think that still begs the question of how CreateBackup() should behave when AwaitCreateBackup() exhausts its polling policy.

It does. One answer is "The same way it does right now". Of course that has the problem of abandoning the operation. For applications where that is a bad idea, at least you can implement your own thing to cancel the operation:

auto op = client.StartCreateBackup(request);
if (!op) return std::move(op).status();
auto operation = *op;
return AwaitCreateBackup(operation).then([o = *std::move(operation)](auto f) {
  auto backup = f.get();
  if (backup) return make_ready_future(std::move(backup));
  CancelOperation(o);
  return AwaitCreateBackup(o);
});

Alternatively, we could add a CancelStalledLongrunningOperationsOption{} that let's one toggle between the current behavior and the one you implemented in #8695

PS: beware ownership of the background threads if implementing any of this.

@devbww
Copy link
Contributor

devbww commented Apr 8, 2022

One answer is "The same way it does right now". Of course that has the problem of abandoning the operation. For applications where that is a bad idea, at least you can implement your own thing to cancel the operation:

Right. I still think non-abandonment should be the default behavior of a CreateBackup(), but I concede that does run the risk of the cancellation request being ignored and the operation taking a long time to succeed/fail.

Alternatively, we could add a CancelStalledLongrunningOperationsOption{} that let's one toggle between the current behavior and the one you implemented in #8695

Yes, perhaps.

@coryan
Copy link
Contributor

coryan commented Aug 25, 2022

I will try to put this in the schedule for 2022-Q4

@coryan
Copy link
Contributor

coryan commented Jan 25, 2023

We do not have time to work on this, closing.

@scotthart
Copy link
Member

This functionality is now present for all LRO methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp: backlog While desirable, we do not have time to work on this for the foreseeable future. cpp: generator Issues related to the C++ micro-generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants