Introduce policy to decide if mutations are idempotent. #64

coryan · 2017-12-01T21:00:18Z

Mutations that are not idempotent are not retried, regardless of whether we have time to do so.

This fixes #13.

coryan · 2017-12-01T21:00:50Z

It looks like 3 commits, but that is just an artifact of the merge, sorry.

mbrukman

@coryan — rather than merging master into your branches or PRs, consider rebasing for cleaner PRs:

git checkout master
git pull upstream master --ff-only

git checkout my-branch
git rebase master

git push -f

This is easy for me to do because I aliased it in ~/.gitconfig:

[alias]
    puma = !git checkout master && git pull upstream master --ff-only

so running git puma does the first two steps.

mbrukman · 2017-12-01T21:03:17Z

bigtable/client/table_apply_test.cc

+    table.Apply(bigtable::SingleRowMutation(
+        "not-idempotent", {bigtable::SetCell("fam", "col", "val")}));
+  } catch (bigtable::PermanentMutationFailures const &ex) {
+    ASSERT_EQ(ex.failures().size(), 1UL);


Expected value should go first (clang-format should fail on this line).

mbrukman · 2017-12-01T21:03:22Z

bigtable/client/table_apply_test.cc

+        "not-idempotent", {bigtable::SetCell("fam", "col", "val")}));
+  } catch (bigtable::PermanentMutationFailures const &ex) {
+    ASSERT_EQ(ex.failures().size(), 1UL);
+    EXPECT_EQ(ex.failures()[0].original_index(), 0);


Expected value should go first (clang-format should fail on this line).

Mutations that are not idempotent are not retried, regardless of whether we have time to do so. This fixes googleapis#13.

coryan · 2017-12-03T13:16:56Z

I rebased this correctly this time. Please take a look.

mbrukman

Some suggestions, some comments and some questions. :-)

mbrukman · 2017-12-03T21:18:55Z

bigtable/client/data.cc

  request.set_table_name(table_name_);
  request.set_row_key(std::move(mut.row_key_));
  request.mutable_mutations()->Swap(&mut.ops_);
+  bool is_idempotent =


mbrukman · 2017-12-03T21:21:15Z

bigtable/client/idempotent_mutation_policy_test.cc

+
+#include "bigtable/client/idempotent_mutation_policy.h"
+
+#include <gmock/gmock.h>


Not sure if you care: this test doesn't seem to use any mocking, so gtest.h should suffice for the EXPECT_(TRUE|FALSE) checks.

I haven't noticed if you just #include <gmock/gmock.h> in every test for consistency; I am guessing GMock includes all of GTest, so this is a superset everywhere. Not a big deal, just curious.

Consistency due to laziness? As far as I know, gmock is a superset, and there are some matches in gmock that can be very useful, e.g., testing::ElementsAre() is defined in gmock

mbrukman · 2017-12-03T21:22:57Z

bigtable/client/table_apply_test.cc

+    table.Apply(bigtable::SingleRowMutation(
+        "not-idempotent", {bigtable::SetCell("fam", "col", "val")}));
+  } catch (bigtable::PermanentMutationFailures const& ex) {
+    ASSERT_EQ(ex.failures().size(), 1UL);


Constant should be the first argument.

mbrukman · 2017-12-03T21:23:01Z

bigtable/client/table_apply_test.cc

+        "not-idempotent", {bigtable::SetCell("fam", "col", "val")}));
+  } catch (bigtable::PermanentMutationFailures const& ex) {
+    ASSERT_EQ(ex.failures().size(), 1UL);
+    EXPECT_EQ(ex.failures()[0].original_index(), 0);


Constant should be the first argument.

mbrukman · 2017-12-03T21:24:25Z

bigtable/client/mutations.h

+/**
+ * Report unrecoverable errors in a partially completed mutation.
+ */
+class PermanentMutationFailures : public std::runtime_error {


I understand that there may be multiple failures that this represents, but it's still weird to see a class named in the plural; I'm used to classes being a singular, but it can contain a list of N failures that happened.

What do you think about making this class name singular?

Good point, thanks!

mbrukman · 2017-12-03T21:26:18Z

bigtable/client/table_apply_test.cc

+    ASSERT_EQ(ex.failures().size(), 1UL);
+    EXPECT_EQ(ex.failures()[0].original_index(), 0);
+  } catch (...) {
+    ADD_FAILURE();


Can this output the exception as a string so that it's visible what failed and why rather than nothing?

I captured the most common exception type and printed it out for that case. In general, there is no portable way to print out exceptions of unknown type. You can raise anything in C++, including void*, or MyClassThatIsNotPrintable. In C++11 you can at least get the current exception as a "thing":

} catch(...) { std::exception_ptr ex = std::current_exception(); ... }

But there is very little you can do with that exception other than raise it and pass it around for somebody else to raise.

coryan · 2017-12-03T21:53:36Z

Thanks for the review! PTAL.

mbrukman · 2017-12-03T22:20:15Z

@coryan — please push your latest changes, I don't see them on GitHub.

mbrukman

LGTM

The change looks good to me; that said, I'd like to get a sign-off from either @garye or @DanielMahu as a sanity-check as well before merging.

coryan · 2017-12-05T13:02:54Z

Ping.

garye · 2017-12-05T20:54:16Z

bigtable/client/idempotent_mutation_policy.h

+  virtual bool is_idempotent(google::bigtable::v2::Mutation const&) = 0;
+};
+
+/// Return an instance of the default MutationRetryPolicy.


Copy and paste error

Oops, fixed.

garye · 2017-12-05T20:55:03Z

bigtable/client/idempotent_mutation_policy.h

+std::unique_ptr<IdempotentMutationPolicy> DefaultIdempotentMutationPolicy();
+
+/**
+ * Implements a safe policy to determine if a mutation is idempotent.


Would be good to clarify what is meant by "safe"

garye · 2017-12-05T20:56:28Z

bigtable/client/mutations.h

+/// Create a mutation to set a cell value where the server sets the time.
+Mutation SetCell(std::string family, std::string column, std::string value);
+
+/// A magic value where the server sets the timestamp.


Can you note here that, if used, the mutation will not be idempotent and therefore retried?

I meant this for the ServerSetTimestamp function:

/// A magic value where the server sets the timestamp (which makes it non-idempotent blah blah blah).
+constexpr std::int64_t ServerSetTimestamp() { return -1; }

coryan · 2017-12-05T22:04:14Z

PTAL.

garye · 2017-12-05T22:07:57Z

bigtable/client/mutations.h

+/// Create a mutation to set a cell value where the server sets the time.
+Mutation SetCell(std::string family, std::string column, std::string value);
+
+/// A magic value where the server sets the timestamp.


I meant this for the ServerSetTimestamp function:

/// A magic value where the server sets the timestamp (which makes it non-idempotent blah blah blah).
+constexpr std::int64_t ServerSetTimestamp() { return -1; }

garye · 2017-12-05T22:08:29Z

bigtable/client/idempotent_mutation_policy.h

+ *
+ * This policy accepts only truly idempotent mutations, that is, it rejects
+ * mutations where the server sets the timestamp.  Some applications may find
+ * this too restrictive and can set their own policies if they wish.


Not a blocker at all but might be nice to have a AlwaysRetryMutationPolicy out of the box

A great suggestion during the PR review.

Another great suggestion from the review.

coryan · 2017-12-05T22:24:37Z

Also fixed the ServerSetTimestamp() function comments.

garye · 2017-12-05T22:35:23Z

bigtable/client/idempotent_mutation_policy.h

  bool is_idempotent(google::bigtable::v2::Mutation const&) override;
 };
+
+/// Implements a policy that retries all mutations.


It would be good to warn the user here about what this means. If a non-idempotent mutation gets retried due a transient error, they will end up with multiple copies of the data with different timestamps. If adding cells, that is.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 1, 2017

coryan requested review from garye and mbrukman December 1, 2017 21:01

mbrukman reviewed Dec 2, 2017

View reviewed changes

coryan added 2 commits December 3, 2017 08:14

Introduce policy to decide if mutations are idempotent.

31de97a

Mutations that are not idempotent are not retried, regardless of whether we have time to do so. This fixes googleapis#13.

Correct formatting using clang-format.

f22d928

coryan force-pushed the fix-issue-13-idempotent-policy branch from 63e52e6 to f22d928 Compare December 3, 2017 13:15

mbrukman suggested changes Dec 3, 2017

View reviewed changes

mbrukman requested a review from DanielMahu December 3, 2017 21:27

Address comments from the review.

1640ee8

mbrukman approved these changes Dec 3, 2017

View reviewed changes

garye reviewed Dec 5, 2017

View reviewed changes

Address comments from the PR review.

dd6d319

garye approved these changes Dec 5, 2017

View reviewed changes

coryan added 2 commits December 5, 2017 17:19

Implement AlwaysRetryMutationPolicy.

7d2749b

A great suggestion during the PR review.

Also fixed the comments for the ServerSetTimestamp().

f274004

Another great suggestion from the review.

garye reviewed Dec 5, 2017

View reviewed changes

Improve docs for AlwaysRetryMutationPolicy [skip ci].

c61d71d

coryan merged commit d1d6b8e into googleapis:master Dec 5, 2017

coryan deleted the fix-issue-13-idempotent-policy branch December 5, 2017 22:49


		#include "bigtable/client/idempotent_mutation_policy.h"

		#include <gmock/gmock.h>

Introduce policy to decide if mutations are idempotent. #64

Introduce policy to decide if mutations are idempotent. #64

Uh oh!

Conversation

coryan commented Dec 1, 2017

Uh oh!

coryan commented Dec 1, 2017

Uh oh!

mbrukman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 commented Dec 3, 2017

Uh oh!

mbrukman 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!

coryan commented Dec 3, 2017

Uh oh!

mbrukman commented Dec 3, 2017

Uh oh!

mbrukman left a comment

Choose a reason for hiding this comment

Uh oh!

coryan commented Dec 5, 2017

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 commented Dec 5, 2017

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 commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrukman left a comment •

edited

Loading