impl(spanner): add support for Read Lock Mode in RW transactions #15235
Conversation
This adds support for Transaction_ReadWriteOptions_ReadLockMode by adding a constructor of ReadWriteOptions with a special enum, whose values can be "optimistic", "pessimistic" and "unspecified".
…uezp/google-cloud-cpp into spanner-read-lock-mode
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15235 +/- ##
==========================================
- Coverage 92.93% 92.92% -0.01%
==========================================
Files 2394 2394
Lines 215352 215411 +59
==========================================
+ Hits 200132 200167 +35
- Misses 15220 15244 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Pending: address issues pointed by checkers-pr and clang-tidy-pr |
| if (UsingEmulator()) { | ||
| GTEST_SKIP() << "Optimistic locks not supported by emulator"; | ||
| } |
There was a problem hiding this comment.
If you look at every other instance of UsingEmulator() in this file you'll find that it is never used to summarily skip a test. Rather, the test is run until the emulator displays different behavior, that result is expected, and then an appropriate action is taken (which might be skipping the remainder of the test). This approach should be maintained as it allows us to actively discover if/when the emulator changes.
| @@ -27,12 +27,14 @@ using ::testing::IsEmpty; | |||
| TEST(TransactionOptions, Construction) { | |||
| Timestamp read_timestamp{}; | |||
| std::chrono::nanoseconds staleness{}; | |||
| auto read_lock_mode = Transaction::ReadLockMode::kOptimistic; | |||
There was a problem hiding this comment.
There's no reason to name this (particularly at this point in the code). Just pass the enumeration value directly to the ctor.
| enum class ReadLockMode { | ||
| kUnspecified, | ||
| kPessimistic, | ||
| kOptimistic, | ||
| }; |
There was a problem hiding this comment.
Suggestion: Do not try to mimic the proto values. Rather, treat this enumeration as abstract, and just map its elements to proto values when filling out the protobuf. Then all the verbiage about proto classes and shorthands can disappear. See spanner::RequestPriority for a similar example.
| @@ -768,6 +770,56 @@ void CheckExecuteQueryWithSingleUseOptions( | |||
| EXPECT_THAT(actual_rows, UnorderedElementsAreArray(expected_rows)); | |||
| } | |||
|
|
|||
| /// @test Verify the `ReadLockMode` option is sent in the RPC by creating a | |||
| /// situation where a transaction A perfomrs a commit while a transaction B | |||
| // transaction A will do a DML after another transaction has executed DML | ||
| // If optimistic, we expect transaction A to be aborted |
There was a problem hiding this comment.
Try to use uppercase letters and periods to begin and end sentences respectively, for clarity's sake (here and below).
| @@ -34,6 +34,13 @@ google::protobuf::Duration ToProto(std::chrono::nanoseconds ns) { | |||
| return proto; | |||
| } | |||
|
|
|||
| google::spanner::v1::TransactionOptions_ReadWrite_ReadLockMode ToProto( | |||
| Transaction::ReadLockMode read_lock_mode) { | |||
| return static_cast< | |||
There was a problem hiding this comment.
See other comment about mapping values so as to avoid casts.
This change is