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

Convert gRPC APIs to use GAPIC #1859

Closed
36 of 40 tasks
stephenplusplus opened this issue Dec 5, 2016 · 23 comments
Closed
36 of 40 tasks

Convert gRPC APIs to use GAPIC #1859

stephenplusplus opened this issue Dec 5, 2016 · 23 comments
Assignees
Labels
api: datastore Issues related to the Datastore API. core

Comments

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Dec 5, 2016

  • Bigtable
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works
  • Datastore
  • Language
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works
  • Logging
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works
  • Pub/Sub
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works
  • Spanner
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works
  • Speech
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works (Not applicable)
  • Vision
    • Generated files are in repo
    • Handwritten API is using generated API
    • Passing through gaxOptions is available on all requests
    • Automatic projectId insertion works (Not applicable)

Cursoring Options

options.autoPaginate

Since the generated API accepts this option, I don't believe we'll need common/paginator. We will simply decorate whatever our callback to gax executes with.

options.maxApiCalls

All API calls come with a cost, and autoPaginate: true leaves that out of the user's control. We added this option to give that control back. Is this supported in the generated layer?

options.maxResults

We support this currently, though I don't know if this is supported in the generated layer. It will simply cut off making further requests after this amount has been reached. So a user can effectively say "autopaginate up until 58 results".

Common Methods

Since each resource will still have some or all of a shared set of methods:

  • delete
  • getMetadata
  • setMetadata

and we can still wrap them with helpers like:

  • create
  • exists
  • get

I believe it will require only a small bit of re-factoring to support, but GrpcService & GrpcServiceObject will likely continue to have a use. Alternatively, it might be possible to use the generic Service and ServiceObject if we can find an acceptable amount of convention/configuration.

common/paginator & common/streamify

Will we need these?

common/util.promisify

Will we need this?

Others

Is there anything else I could be missing?

I came up with this list by trying to migrate the Logging handwritten API to use the generated one. I quickly ran into these bumps, and figured we should resolve these and others up front. Ideally, our repo will remain clean of any artifacts of past implementations, so this will involve a thorough review of the utilities we have in common. @callmehiphop feel free to try a similar migration (pick an API) and see if you uncover any other issues.

// cc @callmehiphop @jmuk @bjwatson

@stephenplusplus stephenplusplus added api: bigtable Issues related to the Bigtable API. core api: datastore Issues related to the Datastore API. api: language Issues related to the Cloud Natural Language API API. api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. api: speech Issues related to the Speech-to-Text API. api: vision Issues related to the Cloud Vision API. labels Dec 5, 2016
@stephenplusplus
Copy link
Contributor Author

/bump after the big week last week :) Some things are @callmehiphop questions, others are @jmuk questions. Please let me know if you need any clarity.

@callmehiphop
Copy link
Contributor

AFAIK we will not need any of the common stuff you mentioned if a library uses GAX, but code changes will be required to return whatever the GAPIC layer returns to the user from the handwritten.

@bjwatson bjwatson added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. Status: Acknowledged labels Mar 2, 2017
@stephenplusplus
Copy link
Contributor Author

@jmuk is it possible to generate the Bigtable and Datastore GAX layer?

@jmuk
Copy link
Contributor

jmuk commented Mar 14, 2017

Datastore will be ready soon. The bigtable API -- it seems we don't have its admin API support yet. You need both normal bigtable and its admin APIs, is that right?

Also cc @lukesneeringer

@stephenplusplus
Copy link
Contributor Author

Correct, both are needed. Thanks for getting Datastore ready!

@stephenplusplus
Copy link
Contributor Author

This issue didn't have enough labels, so I marked it as blocked to reflect the changes we need from google-gax that were uncovered in the Logging conversion PR.

@bjwatson bjwatson changed the title Convert gRPC APIs to use GAX Convert gRPC APIs to use GAPIC Mar 24, 2017
@bjwatson
Copy link

I propose we remove the API labels for the APIs that have already been implemented fully. We don't need this issue coming up in Vision, etc. queries, since it's already done for them.

@stephenplusplus
Copy link
Contributor Author

I'm actually fine with it (in this case!) because it would help us map back to this change (and its goals and discussion), which is helpful in the case we someday find a bug and can track it to this event.

@bjwatson
Copy link

SGTM

@stephenplusplus
Copy link
Contributor Author

I've updated this issue with the task of supporting automatic project ID insertion. This functionality was inadvertently lost when we switched to GAX, and each API that was considered complete will need re-working to support it.

@bjwatson
Copy link

@lukesneeringer @swcloud Friendly reminder to prioritize GAPIC porting work.

@bjwatson
Copy link

@swcloud Ping. I'd like to keep making progress in Q2 on porting manual layers onto GAPIC code, as long as it doesn't interfere with our API deliverables. Stephen and Dave need priorities for that.

@stephenplusplus
Copy link
Contributor Author

To do a re-cap on what this issue is about, each API in the opening post has 4 tasks (defined below):

Generated files are in repo
Is the gapic layer in the repo?

Handwritten API is using generated API
Once the gapic layer made it into the repo, are we using it from the handwritten layer?

Passing through gaxOptions is available on all requests
GAX accepts a group of options that can be things like "autoPaginate" or very fine-grained request settings (deadlines, retry configuration, etc). Are we allowing these settings to be controlled from the user?

Automatic projectId insertion works -- ⚠️BLOCKED⚠️
This is what we're stuck on. The goal is to support automatic project ID detection and insertion into any required request parameters, i.e., creating a resource, like "createTopic({ resource: '{resourceId}' })", where resourceId is a full path "projects/{project}/topics/{topicName}". GAX already has support for detecting {project} when possible (ADC, GCP environments), but what it lacks is a way to automatically insert it into request parameters like these.

The issue for this subject is here: googleapis/gax-nodejs#134

@bjwatson
Copy link

bjwatson commented Jul 10, 2017

Thanks for explaining the current context for this issue @stephenplusplus.

IIUC, the automatic projectId insertion is bound up with this issue raised by @ofrobots: #1891. Correct?

FYI @lukesneeringer

@ofrobots
Copy link
Contributor

This is what we're stuck on. The goal is to support automatic project ID detection and insertion into any required request parameters, i.e., creating a resource, like "createTopic({ resource: '{resourceId}' })", where resourceId is a full path "projects/{project}/topics/{topicName}"

I am worried about automatic text insertion as it could do data corruption if the user data happens to contain strings that happen to match the pattern.

Could we use promises to deal with async initialization, something like:

function promiseOfTopic(topicName) {
  return promiseOfProjectId
      .then(projectId => `projects/${projectId}/topics/${topicName}`);
}

@stephenplusplus
Copy link
Contributor Author

Yes, we can do that, however two things to consider:

@ofrobots
Copy link
Contributor

I am still fairly strongly opposed to string replacement patterns - even with more randomization. We ought to be able to guarantee that we will not clobber user data under any conditions.

@lukesneeringer lukesneeringer removed api: spanner Issues related to the Spanner API. api: bigtable Issues related to the Bigtable API. labels Oct 27, 2017
@lukesneeringer lukesneeringer removed api: speech Issues related to the Speech-to-Text API. api: vision Issues related to the Cloud Vision API. api: language Issues related to the Cloud Natural Language API API. api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. status: acknowledged status: blocked Resolving the issue is dependent on other work. labels Nov 5, 2017
@stephenplusplus
Copy link
Contributor Author

Two APIs remain, Bigtable and Datastore.

  • Bigtable is in its own repo, but does not have a GAPIC API.
  • Datastore is in its own repo, but it is not live. After it goes public and gets a release, the conversion to GAPIC is complete.

@stephenplusplus
Copy link
Contributor Author

Datastore has made the transition, leaving only Bigtable. Issue opened on the Bigtable tracker: googleapis/nodejs-bigtable#23

@stephenplusplus stephenplusplus removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: enhancement labels Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. core
Projects
None yet
Development

No branches or pull requests

6 participants