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

google-cloud-node idioms, for consideration #71

Closed
stephenplusplus opened this issue Nov 21, 2016 · 9 comments
Closed

google-cloud-node idioms, for consideration #71

stephenplusplus opened this issue Nov 21, 2016 · 9 comments
Assignees
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Nov 21, 2016

Originally brought up: googleapis/google-cloud-node#1801 (comment)

google-cloud-node (GCN) has some conventions which ideally could be used here. It would be great to achieve a common UX that can be re-used by a developer when hopping between APIs.

Resource hierarchy

The GCN library layers its classes so that if you need a GCS "Object", for example, you go through 2 parent layers, the GCS class, and then a bucket:

var gcs = require('@google-cloud/storage')({authInfo});
var myBucket = gcs.bucket('my-bucket');
var myFile = myBucket.file('my-file');

myFile.getMetadata(function(err, metadata) {});

I can't speak for certain how this would look with the generated layer, but a simple interpretation would be something like:

var gcs = require('generated-storage-client')({authInfo});

gcs.getObject({
  bucketName: 'my-bucket',
  objectName: 'my-file'
}, function(err, resp) {});

The hierarchy used by GCN is nice because it lets you cache one resource that you intend to make multiple calls with; myFile.delete(), myFile.createReadStream(), etc.

Accessor methods

GCN distinguishes between two types of objects: a "Service" (Google Cloud Storage), and a "ServiceObject" (a Bucket). A consistent set of methods are exposed on a ServiceObject:

  • ServiceObject#create({ config options }, function(err, serviceObjectInstance, apiResponse) {})
  • ServiceObject#delete(function(err, apiResponse) {})
  • ServiceObject#exists(function(err, exists, apiResponse) {})
  • ServiceObject#get(function(err, serviceObjectInstance, apiResponse) {})
  • ServiceObject#getMetadata(function(err, metadata, apiResponse) {})
  • ServiceObject#setMetadata({ new metadata }, function(err, apiResponse) {})

*Methods that don't apply for a specific ServiceObject are removed, e.g. you can't delete a Compute Engine Region, but you can get its metadata.

Streaming methods / naming conventions

The upstream API has its own implementation of logical naming patterns, and in the generated layer, those probably shouldn't be tampered with. However, it might be appreciated by Node.js developers to recognize some names they know from other libraries, for example, createReadStream() and createWriteStream() where there are readable and writable streams.

In GCN's Bigtable API, we expose a createReadStream() to access the proto service's "ReadRows" method.

We've also seen naming conflicts between JavaScript/Node.js definitions and the language-agnostic API terminology. Having a small handwritten map of convenience/conventional names to upstream names might be a big help.

// cc: @bjwatson @jgeewax @callmehiphop @jmdobry

@jgeewax
Copy link

jgeewax commented Nov 21, 2016

👍

@callmehiphop
Copy link
Contributor

In addition to everything @stephenplusplus mentioned, there are a few differences between the gapic layer and GCN.

  • LRO - AFAIK the gapic implementation requires the user to manually poll an endpoint to see if the Operation has completed. In GCN we expose an EventEmitter and perform the polling for the user. This allows them to simply tap into either complete/error events or a Promise resolution.
  • Promises - In GCN we allow the user to provide a Promise module (es6 compatible) if they were prefer to use something like Bluebird instead of native Promises. I do not believe this is configurable in gapic.
  • Retry Logic - It looks like gapic will only retry requests for codes matching either timeouts and unavailable (4, 14). GCN also retries requests for resource exhausted and internal error (8, 13) errors as well.
  • Emulator support - In GCN we accept a customEndpoint parameter for users who wish to use an emulator with the client (which gapic supports via servicePath & port). AFAIK emulators require you to send payloads via plain text which can be done via grpc.credentials.createInsecure(). In GCN if we see that the user is using a custom endpoint, we do this for them. In gapic I believe the user must also generate the creds in addition to specifying the custom endpoint information.

@jmdobry
Copy link

jmdobry commented Nov 21, 2016

LRO - AFAIK the gapic implementation requires the user to manually poll an endpoint to see if the Operation has completed. In GCN we expose an EventEmitter and perform the polling for the user. This allows them to simply tap into either complete/error events or a Promise resolution.

I think this is being addressed in #64

@stephenplusplus
Copy link
Contributor Author

@callmehiphop does this work for the Emulator support point: #46?

@jmuk
Copy link
Contributor

jmuk commented Nov 21, 2016

Basically, we made the design decision that the gapic layers to be close to gRPC definitions (i.e. the same method names + protobuf messages directly). @omaray and @anthmgoogle for the confirmation. cc @bjwatson too.

Because of this, naming changes or restructuring the data structure might conflict with the design policy unfortunately -- and that would require lots of changes on our code generators and codegen configurations (for mapping).

@jmuk
Copy link
Contributor

jmuk commented Nov 21, 2016

Promises

You're right, it's not possible to use other implementations of promises. Filed #72.

Retry Logic

I don't remember why that's only them. @geigerj might remember why those error codes are chosen (I vaguely remember some documentations for which codes should be retryable, but I've lost it).

Emulator support

It is also discussed internally, allow customizing the emulator endpoints as some environment variable IIRC. I haven't heard any updates for that recently, but that will be addressed later.

@callmehiphop
Copy link
Contributor

@callmehiphop does this work for the Emulator support point: #46?

I think it could! Here's how we currently do emulator support

var bigtable = require('@google-cloud/bigtable')({
  customEndpoint: 'a/b/c:8080'
});

I believe the gapic equivalent would be something like

var grpc = require('grpc');

var bigtable = require('@google-cloud/bigtable')({
  servicePath: 'a/b/c',
  port: 8080,
  sslCreds: grpc.credentials.createInsecure()
});

@jmuk jmuk self-assigned this Mar 13, 2017
@jmuk jmuk added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. status: acknowledged status: release blocking labels Mar 13, 2017
@jmuk
Copy link
Contributor

jmuk commented Mar 13, 2017

(note: we should review the discussion again and create issues for individual improvements and fixes)

@jmuk jmuk assigned lukesneeringer and unassigned jmuk Mar 13, 2017
@JustinBeckwith JustinBeckwith added type: cleanup An internal cleanup or hygiene concern. and removed status: acknowledged priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. status: release blocking labels May 31, 2018
@JustinBeckwith
Copy link
Contributor

2.5 years in - I think most of the APIs we have exposed here in gax are pretty settled in sadly. @stephenplusplus @callmehiphop - if we want to make any of this stuff actionable, I would really love it if y'all would create individually actionable bugs that can be picked at one at a time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

7 participants