Google-cloud-node: Return Jobs/Operations directly from the method that starts them

[stephenplusplus]

RE: https://github.com/GoogleCloudPlatform/gcloud-node/pull/1294#discussion_r62872247

Blocked

Let's wait for our support of promises to engineer this.

Before:

vm.delete(function(err, operation, apiResponse) {
  operation
    .on('error', function(err) {})
    .on('running', function(operationMetadata) {})
    .on('complete', function(operationMetadata) {});
});

After:

vm.delete()
  .on('error', function(err) {})
  .on('running', function(apiResponse) {}) // is this what you had in mind, or is it the operationMetadata?
  .on('complete', function(operationMetadata) {});

@callmehiphop What should we do in these situations (regarding the second argument, vm):

compute.createVM('vm-name', function(err, vm, operation, apiResponse) {});

Emit vm with running?

compute.createVM('vm-name')
  .on('error', function(err) {})
  .on('running', function(vm, apiResponse) {})
  .on('complete', function(operationMetadata) {});

... or is that getting weird?

[callmehiphop]

I think if the operation/job was responsible for creating something (like a VM) that the created VM would be emitted via complete.

I forgot about the previously existing running event, perhaps created (or similar) would be better suited.

compute.createVM('vm-name')
  .on('error', function(err) {})
  .on('created', function(apiResponse) {})
  .on('running', function(operationMetadata) {})
  .on('complete', function(vm, operationMetadata) {});

_edit:_ created is bad too, haha, my naming abilities are failing me today

[stephenplusplus]

created and running feel a little too similar, maybe we should just not return the original apiResponse. If an error occurred with the createVM API call, then the apiResponse is already included there. The initial state of the operation shouldn't be vital... but it theoretically should be available from operation.metadata at some point:

var operation = compute.createVM('vm-name')
  .on('error', function(err) {}) // the "POST /vm-name" call failed OR the operation failed
  .on('running', function() {}) // arg removed. `operation.metadata` can be checked for the status
  .on('complete', function(vm) {}); // took out `operationMetadata`. they can check `operation.metadata` for the status

[callmehiphop]

I agree, created was a bad choice, I think the point I'm trying to convey is that we could easily just emit an event specifically to capture the apiResponse. Some other possible options could be queued, operation-created, started, etc..

[stephenplusplus]

I know, I think it's just unnecessary. Please let me know what you think about the example in the last post. I've removed all metadata like arguments from the events themselves.

[callmehiphop]

That could work, I'm not a huge fan of making the initial apiResponse completely inaccessible though.

Unrelated: Does this have any effect on autoCreate apis?

[stephenplusplus]

That could work, I'm not a huge fan of making the initial apiResponse completely inaccessible though.

If it's an error, apiResponse is accessible from the ApiError. If it succeeds, then the response is the initial Job/Operation metadata, which is available from the returned object:

var operation = zone.createVM('vm-name');
operation.metadata === apiResponse; // obviously not immediately, we emit `running` but no potentially stale "status" of the operation

Unrelated: Does this have any effect on autoCreate apis?

Hmm, good thought. Let's see...

var operation = vm.get({ autoCreate: true });

// the "GET /vm-name" call failed OR the "POST /vm-name" call failed OR the operation failed
operation.on('error', function(err) {});

// The VM is being created -- not emitted unless we have to create it
operation.on('running', function() {});

// The VM was either created or it already existed. Return the object here
operation.on('complete', function(vm) {});

Wdyt?

[stephenplusplus]

It does seem like we're stepping on the toes of the future support of Promises... should we wait to tackle this then?

[callmehiphop]

Probably a good idea

[stephenplusplus]

@callmehiphop do you think we should support this in 1.0 or wait until after, at the cost of breaking the API and forcing a 2.0?

// @omaray @jgeewax

[callmehiphop]

I like the idea of supporting it in 1.0 personally, we're already going to be breaking quite a bit with promises, so it might be a good idea to do it now and not make users have to refactor their code a second time when we do get around to it.

[jgeewax]

I agree that this is a pretty broad pattern, so I'd vote we keep this for 1.0 rather than 2.0.

I'm also OK with error, running, and complete. The Operation itself only has a boolean done state, so this seems reasonable.

To translate this to promises, we'd only handle error and complete (as then, and catch) right?

[callmehiphop]

To translate this to promises, we'd only handle error and complete (as then, and catch) right?

Yep!

[jmdobry]

I'm fine with doing this, as long as there's still a way for the user to get back the operation (and not wait for it to complete). I can see scenarios where one would want to start an operation in one place, and wait for it to complete elsewhere (or not wait for it to complete at all).

[stephenplusplus]

Very true. I think that gives us two options...

1. A new method (i.e., createWithOperation())
2. Return a decorated Promise with an event emitter (the Operation)

``` js
var operation = vm.create()

operation.on('running', function(operationMetadata) {
// operation started
})

operation
.then(function(apiResponse) {
// operation completed successfully
})
.catch(function(err) {/*
// POST /instances/new-instance failed OR the operation failed
})
```

[jmdobry]

1. A new method (i.e., createWithOperation())

This could work, we'd need to come up with a consistent naming scheme.

1. Return a decorated Promise with an event emitter (the Operation)

I don't think this would work as the promise would still have the timer that's waiting for the operation to complete. The user needs a way to create the operation without google-cloud-node adding a listener for the complete event of the operation.

e.g.

// Current behavior
speech.startRecognition(...)
  .then((operation) => { ... });

// Behavior proposed by this PR
speech.startRecognition(...)
  .then((transcription) => { ... });

// Option 1: Create operation without waiting, via option
speech.startRecognition(..., { wait: false })
  .then((operation) => { ... });

// Option 2: Create operation without waiting, via different method
speech.startRecognitionNoWait(...)
  .then((operation) => { ... });

[stephenplusplus]

Given that req, I think one new method that returns an operation that doesn't start an interval is the cleanest and clearest separation.

[jmdobry]

Hmm, this is interesting. For the Speech API, the current method name is startRecognition. That name implies starting something, but not necessarily finishing. It's an apt name for a method that returns the operation, no timer started. So, make a new method that calls startRecognition listens for the complete event under the hood?

[jgeewax]

Using the speech example... how about.

// I'd just like to do this the regular way...
speech.recognize(...)
  .then((transcript) => { ... });

// I don't want to wait. Give me the LRO please...
speech.recognize(...)
  .on('start', (operation) => { ... });

// I want the final result, but I also want the operation....
speech.recognize(...)
  .on('start', (operation) => { ... })
  .then((transcript) => { ... });

// The same thing written slightly differently...
speech.recognize(...)
  .on('start', (operation) => { ... })
  .on('success', (operation) => { ... });

I feel like "beginning the work" is an event tied to the technical implementation. Resolving the promise is about completing my "business-level goal" of getting back a transcript of the audio I just shipped up "to the cloud".

In other words... we return a promise (which is an event emitter that happens to spit out either 'success' or 'error'), but this promise also emits other events that are relevant to the underlying implementation details (in this case, "your work has started" via the 'start' event).

---

Source: https://gist.github.com/dmvaldman/12a7e46be6c3097aae31

[stephenplusplus]

@callmehiphop is this feasible to implement? I stumbled on this, which might help: https://github.com/59naga/carrack (via https://npms.io/search?q=promise+eventemitter)

[jmdobry]

I don't see how it's feasible to offer both behaviors without doing it via separate synchronous code paths. The user wants to pick one or the other—start the timer for me (convenience google-cloud-node wants to offer), or don't start the timer for me (how the AsyncRecognize method actually works). @jjg's examples start the timer every time*.

https://github.com/59naga/carrack won't help, as it solves a problem where the timer is always started. If the user wants the operation, then google-cloud-node _cannot_ call .on('start', ...) under the hood at all. Maybe the user doesn't even want to call .on('start', ...), but just wanted to queue the Speech Recognize LRO. Always creating the timer is irresponsible with the user's CPU resources, and unnecessarily queues timers into the event loop, keeping a callstack around in a request/response cycle even if the user's code has already finished executing.

I also don't think we should tell the user to drop to a lower level of abstraction (gapic) to get to the API's default behavior (no timer). We should preserve existing behavior while also adding the new behavior, achieving it via different synchronous code paths (put it behind an option or a new method).

_*The method has to know synchronously whether to start the timer or not (and when it should resolve the promise), it cannot wait for the user to call .then or .on on the method's return value to decide whether to start the timer. Hence the need for an option or new method—these are synchronous signals._

[callmehiphop]

Personally I'm not huge into mixing interfaces like that, I would vote on offering two different methods. I also like the way we handle AsyncRecognize.

My naming could use some work but here's a simple example.

// user doesn't care when the op ends
compute.startCreateVM();

// user cares about the op
var operation = compute.startCreateVM()
  .on('error', console.error)
  .on('whatever', console.log)
  .on('complete', function(vm) {});

// user just cares about when op is over
compute.createVM().then(function(vm) {});

// re using existing operation?
compute.createVM({ operationId: operation.id }).then(function(vm) {});

[stephenplusplus]

@jjgeewax's examples start the timer every time

That doesn't sound right, but let me know if I'm mistaken:

Zone.prototype.createVM = function() {
  console.log('making the request...')
  return this.request(...)
  // ...
}

var operation = zone.createVM();
// => "making the request..."

So if the request can be made when someone calls zone.createVM(), we just need to know when events are registered, so we know what to do next (not sure if this is possible):

// Calling this method makes the API request to create an instance
var operation = zone.createVM();

// Returns the API response from the VM being created
// The operation hasn't started an interval
operation.on('response', function(apiResponse) {});

// Now the operation will start an interval
// This callback executes when the operation is complete
operation.then(function() {
  // VM created
});

Basically, if it's possible to distinguish between when a user registers an event handler on response vs a then handler, we should be able to make this work... maybe?

If not, separate methods are fine with me.

[jmdobry]

On trying to watch for .then: the promise creator is decoupled from the promise consumer(s)—it doesn't know anything about when or how the promise will be consumed. We can't have the promise creator trying to couple itself to the consumer. We'd have to re-implement Promise#then.

On trying to watch for .on: Similar argument as with promise, We'd have to change the implementation of EventEmitter#on in order to hook into it and couple the creator to the consumer. My insides hurt thinking about this.

// Calling this method makes the API request to create an instance
var operation = zone.createVM();

I don't see how Zone#createVM (or Speech#startRecognition) can be synchronous, as they're making an API call. We have to wait for the API to complete to get the operation id in order to even be able to do .on('complete', ...).

@callmehiphop's examples, slightly amended:

// user just wants to enqueue the operation
compute.startCreateVM();

// user cares about the operation
compute.startCreateVM().then((operation) => {
  console.log(operation.id); // id was provided by response from initial API call

  operation
    .on('error', (err) => {
      // error during operation, i.e. server error
    })
    .on('complete', (vm) => {});
}, (err) => { 
  // failed to start operation, i.e. bad argument
});

// user just cares about the result of the operation
compute.createVM().then((vm) => { ... }, (err) => {
  // err could be from initial API call, e.g. failed to create operation
  // or err could have happened during the operation itself
});

[stephenplusplus]

I don't see how Zone#createVM (or Speech#startRecognition) can be synchronous, as they're making an API call.

It wouldn't be synchronous, but using another example, what happens with a method that doesn't use an operation:

Zone.prototype.getVMs = function() {
  return this.request(...);
}

var getVMsPromise = zone.getVMs()
// was a request made, even though I didn't register a `then`?

[stephenplusplus]

Regarding your first two points, I agree that offering dual-functionality from this single method doesn't sound great, but wanted to see if a solution is in fact available, given @jgeewax's original proposal. From a technical standpoint, it might not be as sickening as overriding .then and .on, if there is an underlying event emitter on a Promise, i.e.:

Zone.prototype.createVM = function() {
  var requestPromise = this.request(...)
  requestPromise.on('newListener', function (name) {
    if (name === 'then') // resolve with completed operation
    else if (name === 'response') // resolve with api response
  })
  return requestPromise
}

Just putting this out there in case you guys know if that's how it works. If not, I definitely don't want to override then, on, etc.

[stephenplusplus]

After some thinking, a more simple and obvious solution presented itself, implemented in #1689, which fits in nicely with the chained nature of promises. Feel free to re-open this issue if more discussion is needed.