chore: Pre-DIREGAPIC Refactoring #1 #719

vam-google · 2021-04-22T21:44:07Z

This PR is a minimal subset of #717

This is as minimal of a PR that I could make such that:

Only a single composer is refactored.
It compiles and tests run.
All the key changes are present (abstract/grpc class, comment composer, class names, golden files move)

I had to add a few quite ugly and artificial constructs:

fake ClassComposer under composer package which simply extends composer.common.Composer to void changing the rest of the classes
put back static methods in ClassNames, StubCommentComposer and TestProtoLoaderUtil (for the same reasons as above). All of those chagnes are marked with // TODO: remove after Pre-DIREGAPIC refactoring is fully merged statement.

This is as minimal of a PR that I could make stuch that: 1) It compiles and tests run single composer plus minimal 2) All the key changes are present (abstract/grpc class, comment composer, class names, golden files move) I had to add a few quite ugly and artificial constructs: fake `ClassComposer` under `composer` package which simply extends `composer.common.Composer` to void changing the rest of the classes put back static methods in `ClassNames` and `StubCommentComposer` and `TestProtoLoaderUtil` (for the same reasons as above). All of those chagnes are marked with `// TODO: remove after Pre-DIREGAPIC refactoring is fully merged` statement.

codecov · 2021-04-22T21:53:08Z

Codecov Report

Merging #719 (e81d781) into master (83e2e3b) will increase coverage by 0.18%.
The diff coverage is 81.48%.

❗ Current head e81d781 differs from pull request most recent head a537c93. Consider uploading reports for the commit a537c93 to get more accurate results

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
+ Coverage   92.30%   92.48%   +0.18%     
==========================================
  Files         124      127       +3     
  Lines       13491    13501      +10     
  Branches      996      981      -15     
==========================================
+ Hits        12453    12487      +34     
+ Misses        786      773      -13     
+ Partials      252      241      -11

Impacted Files	Coverage Δ
.../google/api/generator/gapic/composer/Composer.java	`12.67% <ø> (ø)`
...pic/composer/ServiceStubSettingsClassComposer.java	`99.02% <ø> (-0.08%)`	⬇️
...api/generator/gapic/composer/utils/ClassNames.java	`64.00% <0.00%> (-20.22%)`	⬇️
...google/api/generator/gapic/model/GapicContext.java	`94.73% <ø> (ø)`
...google/api/generator/gapic/protoparser/Parser.java	`47.58% <0.00%> (-0.48%)`	⬇️
...le/api/generator/gapic/model/TransportContext.java	`88.88% <88.88%> (ø)`
...api/generator/gapic/composer/grpc/GrpcContext.java	`94.44% <94.44%> (ø)`
...apic/composer/comment/SettingsCommentComposer.java	`93.93% <100.00%> (ø)`
...or/gapic/composer/comment/StubCommentComposer.java	`100.00% <100.00%> (+3.84%)`	⬆️
...r/common/AbstractServiceSettingsClassComposer.java	`97.85% <100.00%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83e2e3b...a537c93. Read the comment docs.

miraleung · 2021-04-22T21:52:00Z

...ava/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java

@@ -110,7 +112,7 @@ public GapicClass generate(GapicContext ignored, Service service) {
            .setName(className)
            .setExtendsType(
                TypeNode.withReference(
-                    FIXED_TYPESTORE
+                    getFixedTypeStore()


Curious - what is the motivation for using a getter if the field is immutable?

Also just basically a best practice thing for java - do not use protected fields, use protected setter/getter for them instead. This field can't be private, because it is accessed in the subclasses, so it must be either protected or have a protected getter, which is what is done here.

Leaving open to revisit after the upcoming changes - if this becomes unneeded by the transport-specific composers, consider making it inaccessible to the subclasses.

miraleung · 2021-04-22T21:55:02Z

...ava/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java

+      TypeStore fixedTypeStore,
+      Class<?> instantiatingChannelProviderClass,
+      String defaultTransportProviderBuilderName) {
+    this.fixedTypeStore = fixedTypeStore;


WDYT about instantiating the intersection of gRPC and REST types in this class's fixedTypeStore? Looks like each call to fixedTypeStore.get() in this file could be a good starter set.

In response to this comment: Good point on sticking with fixedTypeStore.get("Test"). Previously, type existence was guaranteed because fixedTypeStore was instantiated in the same class, which is no longer the case. Hence setting up the common types will address this area.

Sorry I'm not sure I understand your comment instantiating the intersection of gRPC and REST types.

Discussed offline.

src/main/java/com/google/api/generator/gapic/composer/utils/ClassNames.java

src/test/java/com/google/api/generator/test/framework/Differ.java

miraleung · 2021-04-22T22:23:19Z

...ava/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java

+      String defaultTransportProviderBuilderName) {
+    this.fixedTypeStore = fixedTypeStore;
+    this.instantiatingChannelProviderClass = instantiatingChannelProviderClass;
+    this.defaultTransportProviderBuilderName = defaultTransportProviderBuilderName;


Responding to this comment:

To recap, we currently have three possible approaches:

Instantiate any transport-specific values in the transport composer (e.g. GrpcServiceSettingsClassComposer sets defaultTransportProviderBuilderName = "defaultGrpcTransportProviderBuilder" (current approach in this PR).

Advantages: Maximizes per-composer locality.

Drawbacks: Splits per-transport state across many composers, so it may be harder to debug or change transport-specific things. When ClassNames is later involved, there will be another potential set of classes to look up.

Pass the Transport enum into the Abstract class, which can then decide whether to use "Grpc" or "HttpJson" in the string (most suboptimal).

Advantages: Maximizes locality in the Abstract class's common logic (of which there's a significant amount).

Drawbacks: The abstract class should not know anything about per-transport state.

Have a per-transport context object that holds the respective values.

Advantages: Maximizes per-transport locality, transport-specific debugging may be easier.

Drawbacks: Need to look up context in a transport-specific file.

My $0.02: The per-transport context object doesn't have to hold all the state, just anything that needed to be passed into the superclass. Motivation: Passing in differing string values in each per-transport composer class may possibly be less maintainable, especially since the values change within the same transport (e.g. "gRPC" vs "Grpc").

We can also discuss more offline. :)

I can do the contexts, no problem, two questions before doing it (we can discuss it via GVC, just mentioning it here to not forget it):

Please clarify what you mean by context maximizing per-transport locality?

Yes, the context will need to hold the stuff to be passed to the superclass, but there are multiple superlcasses (like 5+) and each of them needs a different set of things (with some intersection, but not much). This means we will either need 5+ different transport specific contexts, or one transport-specific context with stuff shared across different composers in, which will tightly couple the composers class hierarchy with the context and with each other.

Discussed offline.

miraleung · 2021-04-23T17:24:13Z

src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceSettingsClassComposer.java

+    super(
+        createTypes(),
+        InstantiatingGrpcChannelProvider.Builder.class,
+        "defaultGrpcTransportProviderBuilder");


super(InstantiatingGrpcChannelProvider.Builder.class, "defaultGrpcTransportProviderBuilder"); secondFixedTypeStore.add(...);

miraleung · 2021-04-23T17:24:31Z

...ava/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java

+      TypeStore fixedTypeStore,
+      Class<?> instantiatingChannelProviderClass,
+      String defaultTransportProviderBuilderName) {
+    this.fixedTypeStore = fixedTypeStore;


Discussed offline.

Mainly introduce TransportContex and all related stuff.

vam-google · 2021-05-13T09:45:49Z

@miraleung PTAL

miraleung

LGTM. Given the scope of DIREGAPIC's refactorings, could we start a diregapic main branch for all the refactoring PRs, so we can merge that into master when it's all done?

src/main/java/com/google/api/generator/gapic/composer/utils/ClassNames.java

miraleung · 2021-05-13T22:13:07Z

src/main/java/com/google/api/generator/gapic/model/TransportContext.java

+    GRPC,
+    GRPC_REST;
+
+    public static Transport parse(String name) {


Nit: Consider adding a comment with the expected input. As someone familiar with what we're doing, it looks like this will be parsing a Bazel param string like "grpc+rest", so it'd be great to document this method's usage.

Will in the PR which will introduce the actual parameters (here it is just hardcoded everywhere as grpc for now, because unill regapic prs are merged, grpc is the only actually supported transport by the generator.

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.api:gax-bom](https://togithub.com/googleapis/gax-java) | `2.12.0` -> `2.12.2` | [![age](https://badges.renovateapi.com/packages/maven/com.google.api:gax-bom/2.12.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.api:gax-bom/2.12.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.api:gax-bom/2.12.2/compatibility-slim/2.12.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.api:gax-bom/2.12.2/confidence-slim/2.12.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/gax-java</summary> ### [`v2.12.2`](https://togithub.com/googleapis/gax-java/blob/HEAD/CHANGELOG.md#2122-httpsgithubcomgoogleapisgax-javacomparev2121v2122-2022-02-09) [Compare Source](https://togithub.com/googleapis/gax-java/compare/v2.12.1...v2.12.2) ### [`v2.12.1`](https://togithub.com/googleapis/gax-java/blob/HEAD/CHANGELOG.md#2121-httpsgithubcomgoogleapisgax-javacomparev2120v2121-2022-02-09) [Compare Source](https://togithub.com/googleapis/gax-java/compare/v2.12.0...v2.12.1) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).

🤖 I have created a release *beep* *boop* --- ### [2.5.3](googleapis/java-core@v2.5.2...v2.5.3) (2022-02-10) ### Dependencies * update dependency com.google.api:gax-bom to v2.12.2 ([#719](googleapis/java-core#719)) ([8c14b98](googleapis/java-core@8c14b98)) * update dependency com.google.http-client:google-http-client-bom to v1.41.3 ([#720](googleapis/java-core#720)) ([8addf7f](googleapis/java-core@8addf7f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

vam-google requested review from miraleung and a team as code owners April 22, 2021 21:44

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 22, 2021

miraleung reviewed Apr 22, 2021

View reviewed changes

miraleung reviewed Apr 23, 2021

View reviewed changes

vam-google added 2 commits May 13, 2021 02:09

chore: Address PR feedback.

052ffb7

Mainly introduce TransportContex and all related stuff.

chore: Address PR feedback.

e81d781

Mainly introduce TransportContex and all related stuff.

vam-google requested a review from miraleung May 13, 2021 09:45

miraleung approved these changes May 13, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into diregapic_1

a537c93

vam-google changed the base branch from master to diregapic May 18, 2021 09:01

vam-google merged commit 5d182dd into googleapis:diregapic May 18, 2021

vam-google mentioned this pull request May 21, 2021

chore: DIREGAPIC #2 refactoring of the rest Composer Classes #730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Pre-DIREGAPIC Refactoring #1 #719

chore: Pre-DIREGAPIC Refactoring #1 #719

vam-google commented Apr 22, 2021 •

edited

codecov bot commented Apr 22, 2021 •

edited

miraleung Apr 22, 2021

vam-google Apr 23, 2021

miraleung Apr 23, 2021

miraleung Apr 22, 2021

vam-google Apr 23, 2021

miraleung Apr 23, 2021

miraleung Apr 22, 2021

vam-google Apr 23, 2021

miraleung Apr 23, 2021

miraleung Apr 23, 2021

miraleung Apr 23, 2021

vam-google commented May 13, 2021

miraleung left a comment

miraleung May 13, 2021

vam-google May 18, 2021

chore: Pre-DIREGAPIC Refactoring #1 #719

chore: Pre-DIREGAPIC Refactoring #1 #719

Conversation

vam-google commented Apr 22, 2021 • edited

codecov bot commented Apr 22, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented May 13, 2021

miraleung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Apr 22, 2021 •

edited

codecov bot commented Apr 22, 2021 •

edited