Simplify configuration of policies. #809
Conversation
This fixes googleapis#66. Until now, if you wanted to change one of the policies in a Table, TableAdmin, or InstanceAdmin object you had to pass *all* of them in the constructor. With this change you can just pass the ones that you want to override. That makes life easier for our users.
googlebot
added
the
cla: yes
|
@saeta FYI. Let me know if this will not solve your problem with setting just one policy. |
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
==========================================
+ Coverage 95.58% 95.58% +<.01%
==========================================
Files 144 144
Lines 3805 3808 +3
==========================================
+ Hits 3637 3640 +3
Misses 168 168
Continue to review full report at Codecov.
|
| * - `PollingPolicy` for how long will the class wait for | ||
| * `google.longrunning.Operation` to complete. This class combines both | ||
| * the backoff policy for checking long running operations and the | ||
| * retry policy |
There was a problem hiding this comment.
End the sentence with a period? Others (above) do.
| * - `PollingPolicy` for how long will the class wait for | ||
| * `google.longrunning.Operation` to complete. This class combines both | ||
| * the backoff policy for checking long running operations and the | ||
| * retry policy |
There was a problem hiding this comment.
End the sentence with a period? Others (above) do.
looks like this is copied from another file
There was a problem hiding this comment.
Fixed. Yes, it is a copy, I could just reference the documentation in bigtable::InstanceAdmin if you prefer.
There was a problem hiding this comment.
It probably makes sense to keep it where it is so the docs are anywhere someone might see them, so no need to cross-reference, let's keep it as-is.
| } | ||
|
|
||
| template <typename Policy, typename... Policies> | ||
| void ChangePolicies(Policy&& policy, Policies&&... policies) { |
There was a problem hiding this comment.
Nice, I like the functional-style list processing using C++ templates here, I guess this is where && is really useful.
There was a problem hiding this comment.
Indeed, perfect forwarding FTW.
| * - `PollingPolicy` for how long will the class wait for | ||
| * `google.longrunning.Operation` to complete. This class combines both | ||
| * the backoff policy for checking long running operations and the | ||
| * retry policy |
There was a problem hiding this comment.
End the sentence with a period? Others (above) do.
looks like this is copied from another file
| TEST_F(NoexTableTest, ChangeOnePolicy) { | ||
| bigtable::noex::Table table(client_, "some-table", | ||
| bigtable::AlwaysRetryMutationPolicy()); | ||
| EXPECT_THAT(table.table_name(), ::testing::HasSubstr("some-table")); |
There was a problem hiding this comment.
This doesn't really test anything about the policy except that it compiles, right? Should this do a retry on a few (mock) errors to demonstrate the retry policy, or is that overkill?
There was a problem hiding this comment.
Correct, there other test that actually use the policy, for example:
I could create an accessor to read the policy back and figure out if it is of the desired type, but that feels icky. Thoughts?
There was a problem hiding this comment.
No worries, as long as there's an existing test for this, SGTM.
| bigtable::noex::Table table(client_, "some-table", | ||
| bigtable::AlwaysRetryMutationPolicy(), | ||
| bigtable::LimitedErrorCountRetryPolicy(42)); | ||
| EXPECT_THAT(table.table_name(), ::testing::HasSubstr("some-table")); |
There was a problem hiding this comment.
Similarly here: this is not testing either the always-retry-until-N-errors policy. Should that be done here, or is that out of scope?
google/cloud/bigtable/table_admin.h
Outdated
| * - `PollingPolicy` for how long will the class wait for | ||
| * `google.longrunning.Operation` to complete. This class combines both | ||
| * the backoff policy for checking long running operations and the | ||
| * retry policy |
There was a problem hiding this comment.
End the sentence with a period? Others (above) do.
looks like this is copied from another file
There was a problem hiding this comment.
Fixed. Sorry for the comment duplication. This is the only way to make them appear in all the right places for Doxygen.
|
This looks great. Thank you! |
4 participants
This fixes #66. Until now, if you wanted to change one of the
policies in a Table, TableAdmin, or InstanceAdmin object you had
to pass all of them in the constructor. With this change you can
just pass the ones that you want to override. That makes life
easier for our users.