Introduce policy to decide if mutations are idempotent. by coryan · Pull Request #64

Introduce policy to decide if mutations are idempotent. by coryan · Pull Request #64 · googleapis/google-cloud-cpp

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

This fixes #13.

googlebot added the cla: yes

This human has signed the Contributor License Agreement.

label

Dec 1, 2017

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

Choose a reason for hiding this comment

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

@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.

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

Choose a reason for hiding this comment

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

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

		"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);

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

const?

Choose a reason for hiding this comment

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

Yup.


		#include "bigtable/client/idempotent_mutation_policy.h"

		#include <gmock/gmock.h>

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Constant should be the first argument.

Choose a reason for hiding this comment

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

Fixed.

		"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);

Choose a reason for hiding this comment

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

Constant should be the first argument.

Choose a reason for hiding this comment

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

Fixed.

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

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

Good point, thanks!

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Thanks for the review! PTAL.

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

Choose a reason for hiding this comment

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

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.

		virtual bool is_idempotent(google::bigtable::v2::Mutation const&) = 0;
		};

		/// Return an instance of the default MutationRetryPolicy.

Choose a reason for hiding this comment

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

Copy and paste error

Choose a reason for hiding this comment

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

Oops, fixed.

		std::unique_ptr<IdempotentMutationPolicy> DefaultIdempotentMutationPolicy();

		/**
		* Implements a safe policy to determine if a mutation is idempotent.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Done.

		/// 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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Done.

Choose a reason for hiding this comment

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

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; }

		/// 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.

Choose a reason for hiding this comment

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

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; }

		*
		* 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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Yup, done.

A great suggestion during the PR review.

Another great suggestion from the review.

Also fixed the ServerSetTimestamp() function comments.

		bool is_idempotent(google::bigtable::v2::Mutation const&) override;
		};

		/// Implements a policy that retries all mutations.

Choose a reason for hiding this comment

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

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.

coryan deleted the fix-issue-13-idempotent-policy branch

December 5, 2017 22:49