Add GetEntityForUpdate to optimistic and WriteCommitted pessimistic transactions #12668
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D57458304 |
…ransactions (facebook#12668) Summary: The patch adds a new `GetEntityForUpdate` API to optimistic and WriteCommitted pessimistic transactions, which provides transactional wide-column point lookup functionality with concurrency control. Differential Revision: D57458304
|
This pull request was exported from Phabricator. Differential Revision: D57458304 |
…ransactions (facebook#12668) Summary: The patch adds a new `GetEntityForUpdate` API to optimistic and WriteCommitted pessimistic transactions, which provides transactional wide-column point lookup functionality with concurrency control. For WriteCommitted transactions, user-defined timestamps are also supported similarly to the `GetForUpdate` API. Differential Revision: D57458304
|
This pull request was exported from Phabricator. Differential Revision: D57458304 |
…ransactions (facebook#12668) Summary: The patch adds a new `GetEntityForUpdate` API to optimistic and WriteCommitted pessimistic transactions, which provides transactional wide-column point lookup functionality with concurrency control. For WriteCommitted transactions, user-defined timestamps are also supported similarly to the `GetForUpdate` API. Differential Revision: D57458304
|
This pull request was exported from Phabricator. Differential Revision: D57458304 |
| const Slice& key, | ||
| PinnableWideColumns* columns, | ||
| bool exclusive = true, | ||
| bool do_validate = true) = 0; |
There was a problem hiding this comment.
nit: I see other GetForUpdate APIs have const bool do_validate.
There was a problem hiding this comment.
Since do_validate is passed by value, declaring it as const bool vs bool doesn't really make a difference (declaring it const prevents the function's copy of do_validate from being changed in the function body but we don't do that anyway, so to me consts like this just add noise).
|
|
||
| if (!do_validate) { | ||
| if (read_timestamp_ != kMaxTxnTimestamp) { | ||
| return Status::InvalidArgument( |
There was a problem hiding this comment.
Just a thought. Could it be confusing to the users if we ignore do_validate being false and always just force validate when ts is set?
There was a problem hiding this comment.
Yeah, I think silently performing validation when the application passed in do_validate==false might be a bit confusing, especially for users migrating from GetForUpdate to GetEntityForUpdate. So this (and the below) case is where I feel it might make sense to be consistent
| const ReadOptions& read_options, ColumnFamilyHandle* column_family, | ||
| const Slice& key, PinnableWideColumns* columns, bool exclusive, | ||
| bool do_validate) { | ||
| if (!do_validate && read_options.snapshot != nullptr) { |
There was a problem hiding this comment.
Like ts, wondering if we can ignore do_validate being false and always just force validate when snapshot is set.
|
Thanks so much for the review @jaykorean ! |
|
This pull request has been merged in c87f5cf. |
Summary: The patch adds a new
GetEntityForUpdateAPI to optimistic and WriteCommitted pessimistic transactions, which provides transactional wide-column point lookup functionality with concurrency control. For WriteCommitted transactions, user-defined timestamps are also supported similarly to theGetForUpdateAPI.Differential Revision: D57458304