Return Jobs/Operations directly from the method that starts them

@callmehiphop

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?

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

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

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..

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.

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?

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?

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

Probably a good idea

@callmehiphop

@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

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.

@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.

@callmehiphop

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
});

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`?

@jgeewax

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.

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.

chore: update bucket for cloud-rad (#1306)

deps: bump google-gax to latest (#1359)

fix: add close method to Spanner client (#1416)

chore: release 5.11.1 (#1417)

fix: Change image in readme to a static link (#1306)

chore(nodejs): use cloud-rad publication process (#1112) (#1306)

chore: update bucket for cloud-rad (googleapis#1306)

chore(nodejs): use cloud-rad publication process (#1112) (#1306)

Navigation Menu

Repository navigation

Return Jobs/Operations directly from the method that starts them #1306

Blocked

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Return Jobs/Operations directly from the method that starts them #1306

Description

Blocked

Activity

callmehiphop commented on May 11, 2016

stephenplusplus commented on May 11, 2016

callmehiphop commented on May 12, 2016

stephenplusplus commented on May 12, 2016

callmehiphop commented on May 12, 2016

stephenplusplus commented on May 12, 2016

stephenplusplus commented on May 12, 2016

callmehiphop commented on May 12, 2016

stephenplusplus commented on Oct 10, 2016

callmehiphop commented on Oct 10, 2016

11 remaining items

stephenplusplus commented on Oct 11, 2016

jmdobry commented on Oct 12, 2016

stephenplusplus commented on Oct 12, 2016

stephenplusplus commented on Oct 12, 2016

stephenplusplus commented on Oct 14, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions