Skip to content

Conversation

@emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Dec 16, 2022

This PR goes together with GoogleCloudPlatform/spring-cloud-gcp#1392 and builds on top of commits from #1140. It:

  • Refactors the current setup (method-level and individual settings) to use a nested Retry object provided through properties at both service-level and method-level (poc).

    • Previously, configuring granular settings per-method :
    <prefix>.language-service.annotate-text-retry-delay-multiplier=2
    <prefix>.language-service.annotate-text-max-retry-delay=PT0.9S
    
    • With this PR, retry settings will be configured through nested properties with the following precedence order:
      • method-level properties > service-level properties > client library defaults
    // settings provided at service-level will be applied to all rpc methods
    
    <prefix>.language-service.retry.retry-delay-multiplier=2
    <prefix>.language-service.retry.max-retry-delay=PT0.9S
    
    // settings provided at method-level will take precedence over service-level ones configured
    // e.g. annotateText's retry delay multiplier will be set to 3, while max retry delay remains at service-level configuration of 0.9s
    
    <prefix>.language-service.annotate-text-retry.retry-delay-multiplier=3
    

@emmileaf emmileaf added the spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch. label Dec 16, 2022
Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Dropped a small comment and 2 reminder notes below.

.setPakkage(packageName)
.build());

// TODO: This should move to static types once class is added into spring-cloud-gcp-core
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a note here, we will need to load spring-cloud-gcp dependency from local .m2 instead of maven central, since the change is not published yet.
Change will be similar to Blake's commit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhumin8 On a second thought, do you think it would be easier to leave this as a dynamic type in generator code until the shared classes are published and available in maven central?

Let me know if there's anything I'm missing here - currently, this use of dynamic type unblocks code generation, and the generated code is able to compile against a locally installed snapshot jar of spring-cloud-gcp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave as dynamic type temporarily and unblock.
But we do need to switch to local repo for spring-cloud-gcp dependencies eventually to develop on the snapshot version.

currently, this use of dynamic type unblocks code generation, and the generated code is able to compile against a locally installed snapshot jar of spring-cloud-gcp.

yes. But same argument on adding concrete types for other spring denpendencies, it's safer to have the imported class confirmed in the generator than validate via compile in generated module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense. Thank you for the explanation!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@emmileaf emmileaf merged commit d0b45cc into autoconfig-gen-draft2 Dec 20, 2022
@emmileaf emmileaf deleted the spring-refactor-retry-properties-2 branch December 20, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants