Regenerating code, new settings classes

[garrettjonesgoogle]

• Moving settings constants and data into new settings classes
• Making use of ApiCallableBuilder in the new settings classes
• Simplifying the Api classes

[garrettjonesgoogle]

Related PR: googleapis/gax-java#21

[aozarov]

What is the purpose of the baseline folder and why do we have 2 versions of the same class (with same package name). Also, build is broken.

[garrettjonesgoogle]

The baseline folder is to support 3-way merges. This allows us to support hand edits to the main code under src/main/java.

The build is likely broken because it depends on a change in GAX. That is also out for review. I was trying to parallelize.

[garrettjonesgoogle]

FYI I have fixed the build issue by publishing GAX 0.0.3. It looks like the test is still failing intermittently - we have that in our list of things to fix.

[aozarov]

The baseline folder is to support 3-way merges. This allows us to support hand edits to the main code under src/main/java.

Why can't you dynamically generate the based line based on a specific version of the proto (I think generated code should tell from which version it was generated).

[garrettjonesgoogle]

Why can't you dynamically generate the based line based on a specific version of the proto (I think generated code should tell from which version it was generated).

Theoretically this could be done for proto changes, but the other type of change that has to be handled is code generator changes. Those changes can't really be accessed easily. The baseline is a statement of what the generator output for the current file, and thus allows 3-way merges for arbitrary versions of the code built with generators that don't even exist any more.

[garrettjonesgoogle]

I have pushed several changes:

• A number of updates to the surface, e.g. putting the resource helper functions into an inner class
• Using resurrected ServiceApiSettings as a base class for the Settings classes
• Fixed the failing test

PTAL.

[aozarov]

Theoretically this could be done for proto changes, but the other type of change that has to be handled is code generator changes. Those changes can't really be accessed easily. The baseline is a statement of what the generator output for the current file, and thus allows 3-way merges for arbitrary versions of the code built with generators that don't even exist any more.

Honestly, I am not sure I understand the merging model. My understanding was that you want to
allow pulling generated code updates (e.g. if service proto has changes or gax/generation layer has changes) into an existing generated code that was modified. Under this assumption the baseline
could always be generated if gax and proto version that were used to generate the code are known.

Any other module that require users to maintain 2 copies one for use and one for merge feels cumbersome and error prone (I would even question the value of allowing update to the code directly
vs other means as discussed earlier).

In any case, I am not going to push changing gax or the generation layer here and only concern with not breaking the build and making sure our coding standards are adhered.

Build is fine and as this PR does not come to fix the already existing code style issues (which we need to fix later before merge) I am fine with merging this pull request as is.

[garrettjonesgoogle]

Honestly, I am not sure I understand the merging model. My understanding was that you want to
allow pulling generated code updates (e.g. if service proto has changes or gax/generation layer has changes) into an existing generated code that was modified. Under this assumption the baseline
could always be generated if gax and proto version that were used to generate the code are known.

It sounds like you understand it, except that gax doesn't take part in the generation; the code generator is independent. My key point I was trying to make is that the code generator version may actually be difficult to obtain.

Any other module that require users to maintain 2 copies one for use and one for merge feels cumbersome and error prone (I would even question the value of allowing update to the code directly
vs other means as discussed earlier).

The merge process will be taken care of by a codegen pipeline tool, so the user won't ever have to manually mess with the baseline files. I actually think it would be more complex to try to pull in two different versions of the code generator, one of them to generate the baseline, especially if dependencies are different too.

In any case, I am not going to push changing gax or the generation layer here and only concern with not breaking the build and making sure our coding standards are adhered.

Build is fine and as this PR does not come to fix the already existing code style issues (which we need to fix later before merge) I am fine with merging this pull request as is.

I'll remove the newSettings() function and push an update.

What code style issues are you referring to? None of the currently open issues refer to code style, and in the last PR we fixed the Java formatting (we are automatically applying the Google Java code formatter now).

[garrettjonesgoogle]

The change has been pushed. PTAL.

[aozarov]

It sounds like you understand it, except that gax doesn't take part in the generation; the code generator is independent. My key point I was trying to make is that the code generator version may actually be difficult to obtain.

The code generation code is using gax in the generated classes, right? Therefore I think it depends
on it and potentially if gax change the result of the generated code may change, no?

The merge process will be taken care of by a codegen pipeline tool, so the user won't ever have to manually mess with the baseline files

I am not sure I understand this statement. Isn't gcloud-java in this case acts like any other user/client
of the generated code? I assumed that at some point you are going to make the code generation process public and allow users to do it directly (without taking the pre-generated stuff from gcloud-java)
if they wish to. Is my assumption is correct would they not need to apply the same baseline concept?

What code style issues are you referring to?

There are plenty, for start see this:

Starting audit...
PublisherApiTest.java:19:8: error: Unused import: com.google.api.gax.grpc.ApiCallSettings. Use /google/src/head/depot/google3/tools/java/remove_unused_imports.py to automatically remove unused imports.
PublisherApiTest.java:34: warning: Wrong order for org.junit.After import. Use /google/src/head/depot/google3/tools/java/sort_java_imports.py to automatically sort your imports.
PublisherApiTest.java:112: error: Line is longer than 100 characters (found 109).
SubscriberApi.java:36:8: error: Unused import: com.google.api.gax.grpc.ApiCallSettings. Use /google/src/head/depot/google3/tools/java/remove_unused_imports.py to automatically remove unused imports.
SubscriberApi.java:192: error: Line is longer than 100 characters (found 102).
SubscriberApi.java:509: error: Line is longer than 100 characters (found 101).
SubscriberApi.java:631: error: Line is longer than 100 characters (found 102).
SubscriberSettings.java:225: error: Line is longer than 100 characters (found 101).
SubscriberSettings.java:319:7: warning: Name 'LIST_SUBSCRIPTIONS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
PublisherApi.java:36:8: error: Unused import: com.google.api.gax.grpc.ApiCallSettings. Use /google/src/head/depot/google3/tools/java/remove_unused_imports.py to automatically remove unused imports.
PublisherApi.java:189: error: Line is longer than 100 characters (found 101).
testing/LocalPublisherImpl.java:82: error: Line is longer than 100 characters (found 109).
PublisherSettings.java:289:7: warning: Name 'LIST_TOPICS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
PublisherSettings.java:314:7: warning: Name 'LIST_TOPIC_SUBSCRIPTIONS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
SubscriberApi.java:36:8: error: Unused import: com.google.api.gax.grpc.ApiCallSettings. Use /google/src/head/depot/google3/tools/java/remove_unused_imports.py to automatically remove unused imports.
SubscriberApi.java:192: error: Line is longer than 100 characters (found 102).
SubscriberApi.java:509: error: Line is longer than 100 characters (found 101).
SubscriberApi.java:631: error: Line is longer than 100 characters (found 102).
SubscriberSettings.java:225: error: Line is longer than 100 characters (found 101).
SubscriberSettings.java:319:7: warning: Name 'LIST_SUBSCRIPTIONS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
PublisherApi.java:36:8: error: Unused import: com.google.api.gax.grpc.ApiCallSettings. Use /google/src/head/depot/google3/tools/java/remove_unused_imports.py to automatically remove unused imports.
PublisherApi.java:189: error: Line is longer than 100 characters (found 101).
PublisherSettings.java:289:7: warning: Name 'LIST_TOPICS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
PublisherSettings.java:314:7: warning: Name 'LIST_TOPIC_SUBSCRIPTIONS_PAGE_STR_DESC' must match pattern '^[a-z][a-zA-Z0-9]*_?$'.
Audit done.

[garrettjonesgoogle]

It sounds like you understand it, except that gax doesn't take part in the generation; the code generator is independent. My key point I was trying to make is that the code generator version may actually be difficult to obtain.

The code generation code is using gax in the generated classes, right? Therefore I think it depends
on it and potentially if gax change the result of the generated code may change, no?

Yes that is correct. But, the generator can change output without a corresponding GAX change though.

If there is a GAX change, then the generator would change, but there would be a single commit to the target repo (gcloud-java) that bumped up the GAX version plus updated the generated code. If there is no GAX change but there is a generator change, only the generated code changes.

There is another use case here: Iterative development of the code generator. With a baseline file, this is easy. Otherwise, you have to explicitly keep track of each revision of the code generator, even if it isn't checked in locally, in order to do the baselining.

The merge process will be taken care of by a codegen pipeline tool, so the user won't ever have to manually mess with the baseline files

I am not sure I understand this statement. Isn't gcloud-java in this case acts like any other user/client
of the generated code? I assumed that at some point you are going to make the code generation process public and allow users to do it directly (without taking the pre-generated stuff from gcloud-java)
if they wish to. Is my assumption is correct would they not need to apply the same baseline concept?

Yes, gcloud-java is a client of the generated code, and we intend to make the generator public. The generator has the baseline rebasing as part of its complete process. That means when you run the generator, it outputs the newly generated code to a temp directory, then does a 3-way merge with the old baseline files, the newly-generated code, and the existing modified code. You can technically run the generator in isolation, but it's a bit harder because it requires a bunch of inputs that our pipeline is configured to provide. Generally we don't expect anyone to implement the baselining support themselves.

One thing to clarify here is that we originally intended on using a "pull" model for regeneration, where the target repository would know how to regenerate itself. We have since switched to a "push" model, where the pipeline knows how to generate all of our currently supported API/language combinations. It is pretty trivial to target a new or independent repo with the same configuration. When you do that, the generation process will automatically output baseline and src files in the new repo.

What code style issues are you referring to?

There are plenty, for start see this:
...

I have filed issue #756 for this.

[aozarov]

When you do that, the generation process will automatically output baseline and src files in the new repo.

So, generation creates the baseline and depends on it. Hence it is expected from the clients to keep the baseline similar to what we do here?

[garrettjonesgoogle]

When you do that, the generation process will automatically output baseline and src files in the new repo.

So, generation creates the baseline and depends on it. Hence it is expected from the clients to keep the baseline similar to what we do here?

It depends on what you mean by "client" here. We should distinguish two consumers:

1. Repo owner: The owner of the repository holding the generating code needs to check in the baseline with the main src. If they choose not too, then no 3-way merging can be done. If the repo owner does not want to preserve manual edits and always overwrite the generated code, then this is ok.
2. Client library caller: They won't have to be aware that the baselines exist. They are not required for normal compilation, testing, or releasing.

[aozarov]

It depends on what you mean by "client" here. We should distinguish two consumers:

Yes, I am referring to people that are using the generator and plan to modify its output, which I guess
you are referring to as "Repo owner".

Oh, so the generator can work with or without the baseline? If so, why do we need to check-in the baseline now? Do you plan to modify the generated classes? If not, can we add them only before
we plan to apply manual changes to the generated classes (which I doubt we will).

[garrettjonesgoogle]

It depends on what you mean by "client" here. We should distinguish two consumers:

Yes, I am referring to people that are using the generator and plan to modify its output, which I guess
you are referring to as "Repo owner".
Oh, so the generator can work with or without the baseline? If so, why do we need to check-in the baseline now? Do you plan to modify the generated classes? If not, can we add them only before
we plan to apply manual changes to the generated classes (which I doubt we will).

Yes, it can work with or without. I recommend checking in the baseline because we basically can't predict at what point in time we want to do a manual modification. As soon as you want to do a manual modification, you'd have to remember to manually create the baseline, which could be error prone (you might forget, or you might think the generated code has no manual modifications when it actually does, producing an invalid baseline). We wanted to make the process automated and seamless, which would require just letting the generator always keeping them up to date.

[aozarov]

Yes, it can work with or without. I recommend checking in the baseline because we basically can't predict at what point in time we want to do a manual modification

I am not worried about it, as I doubt that we will ever do any significant change to the generated code
AND forget to create a baseline before such that the merge (or fixing it) would be a pain.
However, now that I understand more, I am fine either way.

Actually, I do wonder in that case if generating the baseline as a compressed binary file (e.g. xxx-src.jar) wouldn't be better (less likely to be manually modified, nicer in code review and in general
for delta based versioning systems).

[garrettjonesgoogle]

I am not worried about it, as I doubt that we will ever do any significant change to the generated code
AND forget to create a baseline before such that the merge (or fixing it) would be a pain.
However, now that I understand more, I am fine either way.

Actually, I do wonder in that case if generating the baseline as a compressed binary file (e.g. xxx-src.jar) wouldn't be better (less likely to be manually modified, nicer in code review and in general
for delta based versioning systems).

That's an interesting idea. I'll discuss it with the team. If we decide to do it, I'd prefer to do it in a later PR (and we could remove the baseline files from this PR then).