[DONOT MERGE] showing generated modules #1364

zhumin8 · 2022-11-29T19:39:35Z

This is a draft showing the generated modules language and vision.

global-autoconfig-properties module pending on feat(autoconfig): autoconfig module #1308

From live session:

- review folder/pom structure decisions: do not leak "generated" into maven coordinates Module name changes structure change #1394
- match folder name with artifact id by conventrion
- consider adding to spring-cloud-gcp-dependencies, possibly as a separate BOM
- Add Javadoc to each properties (so Spring can pick up as hints) @emmileaf in fix(spring): add javadoc comments for properties googleapis/sdk-platform-java#1171
- Change “@generated by (gapic-generator-java)” to something more Spring specific fix(spring): change @Generated description to spring specific. googleapis/sdk-platform-java#1103
- Revisit logging strategy in generated code @diegomarquezp
- RetrySetting object - refactored in fix(spring): refactor retry settings to service and method-level nested properties googleapis/sdk-platform-java#1143
- Explore possibility and workload vs. current flatten setup.
- Nice to have, readability is not the top priority of generated code

meltsufin · 2022-11-30T15:58:48Z

generated/language/pom.xml

+    <dependency>
+      <groupId>com.google.cloud</groupId>
+      <artifactId>google-cloud-language</artifactId>
+      <version>2.4.1</version>


I would expect this version to be dependency managed by the parent that imports the libraries-bom.

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

meltsufin · 2022-11-30T16:11:51Z

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

+@EnableConfigurationProperties({LanguageServiceSpringProperties.class, GlobalProperties.class})
+public class LanguageServiceSpringAutoConfiguration {
+  private final LanguageServiceSpringProperties clientProperties;
+  private final GlobalProperties globalProperties;


What are these global properties? I can't find this class.

This class will be brought by #1308

If we are going to have a handwritten shared class, it makes more sense for us to move away from the using the folder name generated.

meltsufin · 2022-11-30T16:18:46Z

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

+      @Qualifier("defaultLanguageServiceTransportChannelProvider")
+          TransportChannelProvider defaultTransportChannelProvider)
+      throws IOException {
+    LanguageServiceSettings.Builder clientSettingsBuilder =


Why not make LanguageServiceSettings its own bean so that it can be overwritten, and then have a separate simple method that takes in LanguageServiceSettings and creates the LanguageServiceClient?

This is a good idea. I'll add to our todo list.

meltsufin · 2022-11-30T16:23:27Z

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

+          LanguageServiceSettings.defaultHttpJsonTransportProviderBuilder().build());
+      LOGGER.info("Using HTTP transport channel");
+    }
+    RetrySettings.Builder analyzeSentimentRetrySettingBuilder =


analyzeSentimentRetrySettings could also be its own bean to allow customization. One thing to remember is that Spring Boot allows you to configure the app via properties, and also by overwriting beans. For the overwriting beans route, you want to make the beans as granular as possible, for maximum flexiblity.

Are you suggesting to have an RetrySetting bean for each RPC? If it has additional benefit other than readability, I think we should do it too.

Yes. For example, it also allows you to use the same retry settings for all RPCs.

meltsufin · 2022-11-30T16:25:09Z

...guage/src/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringProperties.java

+  private String quotaProjectId;
+  private Integer executorThreadCount;
+  private boolean useRest = false;
+  private Duration analyzeSentimentInitialRetryDelay;


As mentioned offline, a lot of duplication here can probably be avoided by creating an object for retry settings.

A helper object sounds like a good idea.

elefeint

LGTM, although you were not planning to merge it in :)

elefeint · 2022-12-02T22:30:27Z

...guage/src/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringProperties.java

+  private String quotaProjectId;
+  private Integer executorThreadCount;
+  private boolean useRest = false;
+  private Duration analyzeSentimentInitialRetryDelay;


A helper object sounds like a good idea.

elefeint · 2022-12-02T22:34:12Z

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

+        .setRetrySettings(classifyTextRetrySettingBuilder.build());
+    RetrySettings.Builder annotateTextRetrySettingBuilder =
+        clientSettingsBuilder.annotateTextSettings().getRetrySettings().toBuilder();
+    if (this.clientProperties.getAnnotateTextInitialRetryDelay() != null) {


@meltsufin this is probably the type of code for which the duplication discussion came up.
IMHO, there are two reasons people read code: one is to modify the code, and the other is to troubleshoot. For the first one one (code modification), there is an obvious benefit to avoiding code duplication ("keeping it DRY") -- if one piece of duplicated code is changed and the other is not, it will lead to bugs. However, this scenario does not apply to autogenerated code, per @blakeli0 's point.
The second scenario -- troubleshooting -- actually benefits from this type of code duplication, as the users can identify the exact set of properties that caused them issues. If duplication were avoided, a loop is needed, so users will then need a conditional breakpoint, which is a bit harder.

elefeint · 2022-12-02T22:48:00Z

generated/language/src/main/resources/META-INF/additional-spring-configuration-metadata.json

+      "description": "Auto-configure Google Cloud language/LanguageService components.",
+      "defaultValue": true
+    }
+  ]


There are probably more options you'd want to enable IDE autocompletion for.

blakeli0 · 2023-01-05T01:27:20Z

...rc/main/java/com/google/cloud/language/v1/spring/LanguageServiceSpringAutoConfiguration.java

+      LOGGER.info(
+          "Background executor thread count is " + this.clientProperties.getExecutorThreadCount());
+    }
+    if (this.clientProperties.getUseRest()) {


I probably missed this earlier, we should use newHttpJsonBuilder in here instead of only setting the transport, newHttpJsonBuilder does a few other things like setting http specific headers. We can move this logic to the top of this method to something like

if (this.clientProperties.getUseRest()) { clientSettingsBuilder = LanguageServiceSettings.newHttpJsonBuilder(); } else { clientSettingsBuilder = LanguageServiceSettings.newBuilder(); }

Thank you for catching this! Put together a quick PoC to note the changes needed, and will be updating the generator code correspondingly.

Updated generator code in googleapis/sdk-platform-java#1227.

zhumin8 · 2023-01-11T21:06:39Z

Please head over to #1449 for a newer version to review. Closing this one.

emmileaf and others added 5 commits November 22, 2022 21:45

WIP: add formatter step on top of pom edits

a486045

WIP: clean up edits unrelated to change

7a37b36

Use fmt-maven-plugin 2.9.1 as latest version with Java 8 support

35eea14

Merge branch 'main' into codegen-include-formatter

a7872b1

add generated modules.

bc69fe0

zhumin8 added the codegen label Nov 29, 2022

zhumin8 requested review from emmileaf, blakeli0, meltsufin and diegomarquezp November 29, 2022 19:40

zhumin8 changed the title ~~[DONOT MERGE] showing generated modules for reviewing~~ [DONOT MERGE] showing generated modules Nov 29, 2022

removing versions that should inherent from parent pom.

9d22b02

diegomarquezp mentioned this pull request Nov 29, 2022

feat: set logging level to trace googleapis/sdk-platform-java#1104

Merged

meltsufin reviewed Nov 30, 2022

View reviewed changes

emmileaf mentioned this pull request Dec 1, 2022

fix(spring): add javadoc comments for configurable properties googleapis/sdk-platform-java#1106

Closed

zhumin8 mentioned this pull request Dec 1, 2022

fix(spring): set autoconfig to be enabled by default. googleapis/sdk-platform-java#1107

Merged

elefeint approved these changes Dec 2, 2022

View reviewed changes

diegomarquezp mentioned this pull request Dec 5, 2022

feat: service settings as a bean googleapis/sdk-platform-java#1110

Merged

This was referenced Dec 6, 2022

poc: refactor retry settings into nested properties class zhumin8/language_poc1#6

Open

poc: refactor retry settings into beans zhumin8/language_poc1#8

Closed

emmileaf mentioned this pull request Dec 20, 2022

fix(spring): add javadoc comments for properties googleapis/sdk-platform-java#1171

Merged

blakeli0 reviewed Jan 5, 2023

View reviewed changes

emmileaf mentioned this pull request Jan 6, 2023

fix(spring): use newHttpJsonBuilder in settings bean for REST transport option googleapis/sdk-platform-java#1227

Merged

zhumin8 closed this Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DONOT MERGE] showing generated modules #1364

[DONOT MERGE] showing generated modules #1364

zhumin8 commented Nov 29, 2022 •

edited

Loading

meltsufin Nov 30, 2022

meltsufin Nov 30, 2022

diegomarquezp Nov 30, 2022

blakeli0 Dec 1, 2022

meltsufin Nov 30, 2022

zhumin8 Dec 1, 2022

meltsufin Nov 30, 2022

blakeli0 Dec 1, 2022

meltsufin Dec 1, 2022

meltsufin Nov 30, 2022

elefeint Dec 2, 2022

elefeint left a comment

elefeint Dec 2, 2022

elefeint Dec 2, 2022

elefeint Dec 2, 2022

blakeli0 Jan 5, 2023

emmileaf Jan 5, 2023

emmileaf Jan 6, 2023

zhumin8 commented Jan 11, 2023

[DONOT MERGE] showing generated modules #1364

[DONOT MERGE] showing generated modules #1364

Conversation

zhumin8 commented Nov 29, 2022 • edited Loading

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

elefeint left a 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

zhumin8 commented Jan 11, 2023

zhumin8 commented Nov 29, 2022 •

edited

Loading