Skip to content
This repository was archived by the owner on Jun 28, 2022. It is now read-only.

AutoValue LongRunningConfig; always use gapic config's polling settings #2698

Merged
merged 2 commits into from
Apr 8, 2019

Conversation

andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Apr 6, 2019

If a non-default GAPIC LRO config is given for a method, always use those polling settings. The gapic config is the only place where LRO polling settings can be defined

metadata and return type are defined by either the protofile's annotations or the GAPIC config.

this reduces the diff between API clients on gapic config v1 and gapic config v2

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 6, 2019
@andreamlin andreamlin changed the base branch from master to gapic_config_v2 April 6, 2019 17:05
@andreamlin andreamlin requested a review from michaelbausor April 6, 2019 17:33
@andreamlin
Copy link
Contributor Author

PTAL

Copy link
Contributor

@michaelbausor michaelbausor left a comment

Choose a reason for hiding this comment

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

LGTM, 1 comment

}

double pollDelayMultiplier = longRunningConfigProto.getPollDelayMultiplier();
if (pollDelayMultiplier <= 1.0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes a failure if pollDelayMultiplier == 1.0, but the comment in the error says that 1.0 is valid - they should be consistent. (I realize this was just taken from below, but let's fix it!)

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #2698 into gapic_config_v2 will increase coverage by <.01%.
The diff coverage is 68.42%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##             gapic_config_v2    #2698      +/-   ##
=====================================================
+ Coverage              86.71%   86.72%   +<.01%     
- Complexity              5585     5587       +2     
=====================================================
  Files                    467      467              
  Lines                  22350    22363      +13     
  Branches                2433     2430       -3     
=====================================================
+ Hits                   19381    19394      +13     
  Misses                  2105     2105              
  Partials                 864      864
Impacted Files Coverage Δ Complexity Δ
...m/google/api/codegen/config/LongRunningConfig.java 55.55% <68.42%> (+7.13%) 14 <7> (+3) ⬆️
.../java/com/google/api/codegen/discovery/Schema.java 84.34% <0%> (-0.51%) 42% <0%> (-1%)

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 d1f2d09...57b29dc. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #2698 into gapic_config_v2 will increase coverage by <.01%.
The diff coverage is 68.42%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##             gapic_config_v2    #2698      +/-   ##
=====================================================
+ Coverage              86.71%   86.72%   +<.01%     
- Complexity              5585     5587       +2     
=====================================================
  Files                    467      467              
  Lines                  22350    22363      +13     
  Branches                2433     2430       -3     
=====================================================
+ Hits                   19381    19394      +13     
  Misses                  2105     2105              
  Partials                 864      864
Impacted Files Coverage Δ Complexity Δ
...m/google/api/codegen/config/LongRunningConfig.java 55.55% <68.42%> (+7.13%) 14 <7> (+3) ⬆️
.../java/com/google/api/codegen/discovery/Schema.java 84.34% <0%> (-0.51%) 42% <0%> (-1%)

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 d1f2d09...57b29dc. Read the comment docs.

@andreamlin andreamlin merged commit 317edce into googleapis:gapic_config_v2 Apr 8, 2019
@andreamlin andreamlin deleted the lro_merge branch April 8, 2019 16:41
andreamlin added a commit that referenced this pull request Apr 22, 2019
* Add Gapic config v2 (#2665)
* Whittling down config_v2 (#2666)
* Add ConfigV2 Validator (#2672)
* AutoValue LongRunningConfig; always use gapic config's polling settings (#2698)
* ResourceNameOneofConfig fixes (#2704)
* Start parsing GAPIC config v2 (#2703)
* Bring back timeout millis in GAPIC config v2 (#2708)
* Resource names across different protofiles (#2711)
* Fix missing default retries (#2718)
* Bug fixes for gapic config v2 parsing (#2717)
busunkim96 pushed a commit to busunkim96/gapic-generator that referenced this pull request Nov 7, 2019
* Add Gapic config v2 (googleapis#2665)
* Whittling down config_v2 (googleapis#2666)
* Add ConfigV2 Validator (googleapis#2672)
* AutoValue LongRunningConfig; always use gapic config's polling settings (googleapis#2698)
* ResourceNameOneofConfig fixes (googleapis#2704)
* Start parsing GAPIC config v2 (googleapis#2703)
* Bring back timeout millis in GAPIC config v2 (googleapis#2708)
* Resource names across different protofiles (googleapis#2711)
* Fix missing default retries (googleapis#2718)
* Bug fixes for gapic config v2 parsing (googleapis#2717)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants