Initial generated gcloud-java-pubsub API and GAX

[garrettjonesgoogle]

No description provided.

[jgeewax]

/cc @mziccard -- Thanks @garrettjonesgoogle !

[aozarov]

/cc @tmatsuo

[tmatsuo]

Great to see it's coming. Have some questions.

• What does GAX stand for?
• The size of the change exceeds the threshold of comfortably reviewing the code on github. Are there any other options?
• However, most of the files are just auto-generated and no need for review, please state so in the description, which might be helpful for reviewers.

Thanks!

[jgeewax]

@tmatsuo : This code is auto-generated by an internal team hoping to give us a bunch of tools to use. This was clear to us (they sent us an e-mail beforehand).

[garrettjonesgoogle]

I have a few things to mention:

• The stuff under gcloud-java-gax is going to move to a separate repository in the near-ish future, so gcloud won't hold it for too long.
• Pubsub doesn't yet have grpc turned on externally, so the code can't be tested against the real service yet.
• The List* functions don't work (even against a version of pubsub with grpc turned on) - we need to do some extra work for the page streaming pieces to work.

[aozarov]

Great! I will look at this PR more carefully tomorrow.

Few questions/comments:

What is stopping us from moving gax into its own gitHub repository sooner than later?
Such setup should not be a big deal.
Also, I wonder in the case of GAX if you really want to check-in the generated part or can
that be generated in the build?

Because PubSub is not yet public I think this PR should be against a dedicate branch and not master (as people expect anything in master to be fully functional).

Also, this PR is too big for GitHub to work correctly. e.g. it can't show the full file list, or I can't
even comment on files as I can only see them in "View" mode (which does not allow adding comments). I was about to add a comment in the gcloud-java-pubsub/README.md saying that some links are broken and it does not conform to the new page style...

As for the service specific generated code, I think we did say before that we would check them in but
I think it would be good to publicly (e.g. here) explain why we prefer that over dynamic generation based on the service proto (which could be checked in or retrieved as a maven dependency).

I also find the src/main/java vs src/generated/main/java distinction a bit strange, as also the files
under src/main/java are generated.

So far, services also included a package-info.java with a "how-to-use-this-service" example, do you think this is something that you can generate as well?

As for the generated code, it looks like "@param" wrapping is not using the google-style.
amd Javadoc does not include "@return" or "@throws".
Also, I am not sure the ServiceApi classes are easily testable, considering their private constructor
and no public interface. BTW, so far for all gcloud-java classes we scope them under "com/google/gcloud" package.

Overall, I think we should treat the ServiceApi classes as spi files (e.g. gcloud-java-pubsub/src/main/java/com/google/gcloud/spi/v1/SubscriberApi.java) and provide a hand-crafted APIs that will be the user-facing and use the common setup mode (using gcloud-java-core).

[garrettjonesgoogle]

/cc @anthmgoogle

[garrettjonesgoogle]

• We are going to put GAX into its own org, and we'll need to go through the open source approval process. We didn't want to block the pubsub effort on that.
• We know how to generate the GAX code using gradle, and we already have that working internally, and that's what we'll use in our own repository. It doesn't seem worth the effort to get it working under maven (if that's possible, we're not sure) for the short term.
• I can switch the PR to a different branch and break it apart. What is github's limit?
• Checking in service-specific generated code: This is to allow for manual additions to the generated code, and also short-term edits for expediency.
• It is true that files under src/main/java are generated, but they also take manual edits, so they aren't purely generated either.
• I do have some example code to use this wrapper, but it doesn't run yet (due to the issues named earlier), which is why I held back on submitting it. I could put some example code into a package-info.java file though.
• I will look into the @return and @throws parts of the javadoc.
• I have a separate work item to figure out how to test the wrapper. I have been looking at mocking something in the grpc level.

[anthmgoogle]

This submission is clearly very large. As a first step, let's separate the size challenges, I think it is a reasonable request to break this down into:

1. gRpC code-gen. This either needs to be dynamically generated or to be a static copy. The proposal in the near term is that this is static, so it includes a static copy of the code-gen that is api-specific for pub-sub, but also the transitive closure of dependencies. The reasoning is that this should be stable enough in the short term, and this drastically simplifies the run-time dependencies. The plan is to make this dynamic going forward.
2. GAX or Google API eXtensions. This is the static library that the code-generation depends on. The medium term plan is that this will be live in its own GitHub repo. In the short term, JJ told us to go ahead and check it in to the gcloud-java repo to unblock pubsub.
3. The wrapper code gen is what our new tooling has produced. At minimum this should be a building block. We should discuss if the "spi" namespace is appropriate.

[anthmgoogle]

To drill more into the "spi" namepace, it would be good to understand what that is meant for.

The code generated layer is intended to have good usability in its own right. It should allow for dropping down to direct gRPC use in advanced cases.

[garrettjonesgoogle]

@aozarov / others from the team - could you address these points?

• Are you ok with allowing the gax component of this PR as-is, i.e. using the src/main/java vs src/generated/main/java pattern, and checking in the generated code instead of generating it in the build? (Just to reduce the churn on this piece, since it is a short-term thing)
• I looked up github's limit, and the limit we seem to have hit was the 10k lines changed limit (https://help.github.com/articles/what-are-the-limits-for-viewing-content-and-diffs-in-my-repository/). I was thinking of splitting the PR this way:
  A. "low-scrutiny" PR's:
  1. PR for GAX protoc-generated code
  2. PR for GAX handwritten code
  3. PR for protoc & grpc generated pubsub code
     B. "high-scrutiny" PR:
  4. PR for ServiceApi generated classes

Does this sound good?

• We can consider changing the src/main/java vs src/generated/main/java pattern for the pubsub piece; do you have any specific suggestions?
• Do you consider the non-testability of the ServiceApi classes a blocker for checking in the first iteration?
• Can you explain the spi namespace?

[aozarov]

@garrettjonesgoogle

Are you ok with allowing the gax component of this PR as-is, i.e. using the src/main/java vs src/generated/main/java pattern, and checking in the generated code instead of generating it in the build? (Just to reduce the churn on this piece, since it is a short-term thing)
Sure, if it helps to get things forward faster. Do we agree that in some point later that would be moved to its own repository and consumed as a maven artifact by gcloud-java?

Your PR split proposal sounds fine to me.

Though I would like all of your changes to be in a branch I wonder if the "temporary" gax parts could go to master as they are not consumable by clients directly and in order to avoid the same problem when trying to merge the branch to master...

The spi namespace basically suggests that this code is a lower-level gateway to the service however not recommended to be directly used by clients. The client facing API, which delegates the calls to the spi, is a consistent, simplified, testable and highly documented part of the library that is also go through a much more strict backward compatibility policy.
I suggest to take a look at storage and/or datastore modules of this repository to get more clarification on the distinction.

In its current state it does not look like the service specific parts match the criteria mentioned above, but I was not sure if that was the plan (as we did discuses the options of wrapping the generated code with a thin hand-crafted layer). @jgeewax what is your take on it?
I think agreeing on this part would clarify and help to resolve some of the other issues mentioned.

Honestly, I think it would be nice if all services (generated or not) share the same setup code (part
of gcloud-java-core). That part already takes care of finding default project and authentication for
AE, GCE and other.

I do think non-testable API is a blocker but that should not be an issue if it lives in a development branch.

BTW, is there some public documentation on Google API eXtension?

Also, I could not see in ServiceApiSettings any options to configure timeout/deadline should
I be looking in a different place?

[jgeewax]

In its current state it does not look like the service specific parts match the criteria mentioned above, but I was not sure if that was the plan (as we did discuses the options of wrapping the generated code with a thin hand-crafted layer).

@aozarov : This is a super long thread. Can you be more specific about which criteria above we're talking about here?

[jgeewax]

Just to be clear to everyone regarding the purpose of this code....

Purpose of this auto-generated stuff

For cloud client libraries (ie, gcloud-java), this auto-generated code is meant as a tool for the library maintainers (aka, if you write code that lives in the gcloud-java repository).

If consumers of gcloud-java want to use auto-generated code (or gRPC code) directly (ie, some super advanced case), we shouldn't stop them, however, the auto-generated methods won't be advertised widely as "the way to use gcloud-java".

In short, anyone using these methods directly should be intimately familiar with the source code of gcloud-java, not just how to use it based on our API documentation.

Broader scope: beyond Google Cloud Platform

For non-cloud client libraries (ie, Maps? in the future?), auto-generated code like this is intended to be used directly by customers, as those services don't have a team (like us) working on the client library.

The team working on auto-generating this code has a goal to make it more user-friendly (better method naming, more closely fitting to the language, better documentation), and we will likely benefit from that.

Regardless of how good the auto-generation gets, there are debates about API surfaces that have no right answer, which means we will continue to own and curate the design of the API surface, and can hopefully benefit from auto-generated code doing some of the heavy lifting for us.

This pull request

I requested that the code be sent to us as a pull-request rather than as a dependency so that we could modify the auto-generated code if we needed to, rather than our changes being blocked by pull requests, merges, and then releases before we could get what we wanted from the code.

If the auto-generated code settles to the point where we feel changes are extraordinarily rare, we should discuss breaking it out into it's own repository.

That said, the size of the PR is larger than I expected, so we should try to get an idea of what auto-generated code we expect to need to modify and what code we won't. Anything that falls under the "it's unlikely we'll need to change it" should be kicked off to a separate repository and treated like any other dependency.

/cc @aozarov @mziccard @garrettjonesgoogle @anthmgoogle

[aozarov]

@aozarov : This is a super long thread. Can you be more specific about which criteria above we're talking about here?

Mentioned once sentence above "consistent, simplified, testable and highly documented part of the library that is also go through a much more strict backward compatibility policy".

In this PR, issues with "testable" and "highly documented" are clearly visible, but the I think also the lack
of consistency with the other hand-crafted services (e.g. initial setup and usage patterns) is a problem.

However, based on your comment,"Purpose of this auto-generated stuff", I think this case falls exactly in the bucket of our spi layer, which though can be used by clients directly, is not exposed by the main api
and does not adhere to the same standards (e.g. documentation, backward compatibility,..).
In that case, I think the package structure should be changed and some of the things I mentioned earlier
(such as missing a package-info.java) are no longer an issue.

[anthmgoogle]

The biggest part of this is the gRPC codegen for API-independent dependencies (e.g. the java codegen from timestamp.proto). We measured this to be about 2.4Mb in binary form. This is required to use gRPC one way or another. This part could potentially consumed as a dependency instead of source, although we don't have the repository ready to do that right now. If this part is not too big, it would be simplest if we could include it as source in the short term, and switch to either a dependency or dynamic generation later.

[garrettjonesgoogle]

Actually it is 2.4Mb in source form, and 767k in compiled jar form.

[mziccard]

Just giving my two cents. First, thanks for this PR as it's going to make our job much easier on pub-sub.
What I know/understood:

• You can not move (part of) the generated code to its own repository/artifact yet
• Looking at the code I expect we will not need to touch the GAX part but I still don't know if we'll need to change the pub-sub generated code

What I suggest:

• PR splitting proposed by @garrettjonesgoogle sounds good to me
• Merge everything (even the GAX part) to a different branch: so that we can move/rename/experiment and do whatever without affecting the master branch (and Travis) or possible contributors.
• Considering all the pub-sub code in this PR as what we call spi (this implies a package renaming): this should allow us to build our (thin) layer on top of it following gcloud-java conventions and practices

What I expect from the future:

• You pushing other PRs to if the generated code changes
• Us providing you feedback on what we added to/would like to have in the generated code
• As soon as we converge to a stable spi layer (no changes on either sides) moving all that should not be in gcloud-java (I am thinking of GAX here) to a different repo+artifact

Question:

• Do we need to (try to) test generated code? Can we consider it as trustworthy and rely solely on integration tests?

Thoughts? Sorry for the long post:)

[garrettjonesgoogle]

I created a branch pubsub-alpha in my fork, but I don't see a way to submit a pull request to the equivalent branch name in GoogleCloudPlatform (it doesn't exist there yet) - do you guys need to create it first?

[aozarov]

I created one for you. same name pubsub-alpha. Even on this branch lets split the PR as you suggested in the proposal.

[garrettjonesgoogle]

For those watching, I created the first pull to pubsub-alpha with the generated proto classes: #311

[aozarov]

Closing this PR in favor of chunking it into smaller PRs (some of which such as ##320 and #317were already merged).

However, this thread still have few unanswered questions such as:

• timeout/deadline handling
• testing of generated code