Set gapic limits for retries by manish-qlogic · Pull Request #1030 · googleapis/google-cloud-cpp

manish-qlogic · 2018-08-24T01:47:03Z

This fixes#428 .

This change is

codecov · 2018-08-24T02:27:57Z

Codecov Report

Merging #1030 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #1030 +/- ##
==========================================
+ Coverage 94.62% 94.62% +<.01% 
==========================================
Files 183 183 Lines 8002 8005 +3 ==========================================
+ Hits 7572 7575 +3 
Misses 430 430

Impacted Files	Coverage Δ
google/cloud/bigtable/rpc_backoff_policy.h	`100% <ø> (ø)`	⬆️
google/cloud/bigtable/rpc_retry_policy.h	`100% <ø> (ø)`	⬆️
google/cloud/bigtable/rpc_backoff_policy.cc	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/rpc_retry_policy.cc	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/internal/table.h	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/internal/table_admin.h	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/polling_policy.cc	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/polling_policy.h	`100% <100%> (ø)`	⬆️
google/cloud/bigtable/internal/instance_admin.h	`83.6% <100%> (ø)`	⬆️

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 942e8e1...0fa2ddb. Read the comment docs.

coryan · 2018-08-24T16:39:17Z

+// AUTOGENERATED BY tools/generate_rpc_policy_defaults.py; DO NOT
+// EDIT MANUALLY. ALL MANUAL CHANGES WILL BE OVERWRITTEN.
+//
+// Copyright 2018 Google Inc.


s/Inc./LLC/

coryan · 2018-08-24T16:42:04Z

+import sys
+import urllib
+
+FILE_HEADER = """


We probably want all of this in a static file, take a look at:
https://github.com/GoogleCloudPlatform/google-cloud-cpp/blob/master/google/cloud/bigtable/internal/readrowsparser_acceptance_tests.inc
All the code that does not need to be generated is in a normal file, which includes the generated code:
https://github.com/GoogleCloudPlatform/google-cloud-cpp/blob/c313463e06cf385525b9cd0eeb0d2467bf185e7c/google/cloud/bigtable/internal/readrowsparser_test.cc#L251
Could you do that here to make the generator (and the generated code) smaller?

coryan · 2018-08-24T16:45:04Z

+inline namespace BIGTABLE_CLIENT_NS {
+namespace internal {
+
+struct RPCPolicyDefaults {


I think this class represents the RPCPolicyParameters, they are not the defaults because we no longer have defaults for the policy (we have default policy parameters for each class).

renamed to RPCPolicyParameters.

coryan · 2018-08-24T16:46:18Z

+ std::chrono::milliseconds(60000)
+#endif // BIGTABLETABLEADMIN_DEFAULT_MAXIMUM_DELAY
+
+const struct RPCPolicyDefaults kBigtableTableAdminLimits = {


If my suggestion above makes sense, this would become:
RPCPolicyParameters constkBigtableTableAdminDefaults{

coryan · 2018-08-24T16:46:34Z

+ std::chrono::milliseconds(60000)
+#endif // BIGTABLETABLEADMIN_DEFAULT_MAXIMUM_DELAY
+
+const struct RPCPolicyDefaults kBigtableTableAdminLimits = {


Why do you need an explicit struct here?

coryan

A lot of formatting nits but otherwise looks good.

coryan · 2018-08-25T15:37:25Z

+// AUTOGENERATED BY tools/generate_rpc_policy_parameters.py; DO NOT EDIT
+// MANUALLY. ALL MANUAL CHANGES WILL BE OVERWRITTEN.
+//
+// Copyright 2017 Google LLC


s/2017/2018/

coryan · 2018-08-25T15:38:42Z

+#include "google/cloud/bigtable/internal/rpc_policy_parameters.h"
 #include "google/cloud/bigtable/version.h"
 #include "google/cloud/internal/retry_policy.h"
+


Remove the blank line please.

blank line removed.

coryan · 2018-08-25T15:39:05Z

+// AUTOGENERATED BY tools/generate_rpc_policy_parameters.py; DO NOT EDIT MANUALLY.
+// ALL MANUAL CHANGES WILL BE OVERWRITTEN.
+//
+// Copyright 2017 Google LLC


s/2017/2018/

coryan · 2018-08-25T15:40:20Z

+// the application developers to change the defaults for their
+// application by compiling with different values.
+
+#ifndef BIGTABLEINSTANCEADMIN_DEFAULT_INITIAL_DELAY


I think these macros should be prefixed by GOOGLE_CLOUD_CPP_, they get very long, but that seems safer.

Added prefix.

coryan · 2018-08-25T15:41:23Z

+ BIGTABLE_DEFAULT_INITIAL_DELAY,
+ BIGTABLE_DEFAULT_MAXIMUM_RETRY_PERIOD,
+ BIGTABLE_DEFAULT_MAXIMUM_DELAY,
+};


Consider adding a#undef for each one of the macros at the end of this file. Once used to define the C++ constants they are no longer needed and we should avoid polluting the application namespace with macros.

#undef added at the end of file.

coryan · 2018-08-25T15:54:12Z

+
+""" Generate header file for retry policy defaults from GAPIC yamls
+
+GAPIC suggests default values for clients to design their retry policy.


Maybe:
GAPIC defines default values for the retry policies in the client library.

coryan · 2018-08-25T15:54:28Z

+For Bigtable data APIs it is defined in bigtable_gapic.yaml
+https://raw.githubusercontent.com/googleapis/googleapis/master/google/bigtable/v2/bigtable_gapic.yaml
+
+This script reads both yamls and generates a C++ header file that define


s/yamls/YAML files/

coryan · 2018-08-25T15:55:10Z

+https://raw.githubusercontent.com/googleapis/googleapis/master/google/bigtable/v2/bigtable_gapic.yaml
+
+This script reads both yamls and generates a C++ header file that define
+constants which is used in retry and backoff policies for Bigtable Clients.


s/Bigtable Clients/Cloud Bigtable C++ Client Library/

coryan · 2018-08-25T15:56:44Z

+ print prefix + constant + ","
+ print "};"
+
+def print_test(t):


I think the Python style guide(s) requires two blank lines before a function or class definition. You could use yapf to reformat this file.

used yapf to format python script.

coryan · 2018-08-25T15:57:26Z

+def print_defines(interface):
+ prefix = interface['name'].upper().rsplit('.',1)[1]
+ struct = interface['name'].rsplit('.',1)[1]
+ constants = {"_DEFAULT_INITIAL_DELAY":"initial_retry_delay_millis",


I think the python style guide(s) require a space after the :, but yapf will fix this for you as suggested below.

used yapf to format python script.

manish-qlogic · 2018-08-26T08:26:39Z

@coryan,

How to include generated file in the build, builds with TEST_INSTALL=yes are failing as build is unable to locate rpc_policy_parameters.inc.

Inclusion of rpc_policy_parameters.inc in google/cloud/bigtable/CMakeLists.txt does not trigger changes in bazel file. Do we need some special processing for rpc_policy_parameters.inc too?

coryan · 2018-08-26T13:57:57Z

@manish-qlogic I think you need to change cmake/CreateBazelConfig.cmake to treat .inc the same way we treat .h files in the Bazel files.

You also need change google/cloud/bigtable/CMakeLists.txt around line 370 to something like this:

install(DIRECTORY . DESTINATION include/google/cloud/bigtable
FILES_MATCHINGPATTERN"*.inc"FILES_MATCHINGPATTERN"*.h"PATTERN"testing/*"
EXCLUDE)

Check the documentation because I am not sure of the syntax.

manish-qlogic · 2018-08-27T03:47:20Z

PTAL

coryan · 2018-08-27T14:30:56Z

Can you do a rebase to master one more time? I would like to verify the examples run correctly with your changes (they should, but still would like to check).

manish-qlogic · 2018-08-27T23:22:45Z

Rebased it, PTAL

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2018

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 24, 2018

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 24, 2018

coryan requested changes Aug 24, 2018
View reviewed changes

coryan requested changes Aug 25, 2018
View reviewed changes

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 25, 2018

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 25, 2018

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2018

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2018

manish-qlogic added 5 commits August 27, 2018 20:34

admin adn data client uses GAPIC limits as defaults for retry policies
96eda9b

generating rpc_policy_defaults under appropriate version
0f53e14

renaming rpc_policy_defaults to rpc_policy_parameters
bef3903

resolving formatting nits
ba2f37f

bazel now treats .inc files as header files
0fa2ddb

manish-qlogic force-pushed the set_GAPIC_limits_for-retries branch from b82e01f to 0fa2ddbCompare August 27, 2018 15:06

coryan added the api: bigtable Issues related to the Bigtable API. label Aug 27, 2018

coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2018

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2018

coryan approved these changes Aug 28, 2018
View reviewed changes

coryan merged commit 75d449c into googleapis:masterAug 28, 2018

manish-qlogic deleted the set_GAPIC_limits_for-retries branch September 8, 2018 09:14


		""" Generate header file for retry policy defaults from GAPIC yamls

		GAPIC suggests default values for clients to design their retry policy.

Uh oh!

Conversation

manish-qlogic commented Aug 24, 2018• edited by coryan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Aug 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manish-qlogic commented Aug 26, 2018

Uh oh!

coryan commented Aug 26, 2018

Uh oh!

manish-qlogic commented Aug 27, 2018

Uh oh!

coryan commented Aug 27, 2018

Uh oh!

manish-qlogic commented Aug 24, 2018•
edited by coryan
Loading

codecovBot commented Aug 24, 2018•
edited
Loading