Adds support for atomic transactions spanning multiple KV entries. #2028
Conversation
|
|
||
| // Txn is used to apply multiple KV operations in a single, atomic transaction. | ||
| // Note that Go will perform the required base64 encoding on the values | ||
| // automatically because the type is a byte slice. |
There was a problem hiding this comment.
I know you're working on docs here, but can you include a small example snippet for how the API would use a transaction here in the comment block?
| // automatically because the type is a byte slice. Transactions are defined as a | ||
| // list of operations to perform, using the KVOp constants and KVTxnOp structure | ||
| // to define operations. If any operation fails, none of the changes are applied | ||
| // to the state store. |
There was a problem hiding this comment.
What is the locking protocol used? Suppose txn A updates keys 1 and 2, and transaction B updates keys 3, 2, and 1. When executing the transaction, txn B updates 3, updates 2, and the thread is put to sleep. Txn A comes through and completes its updates to keys 1 and 2. Will the API replay the txn or will it outright fail because there was a concurrent update that caused txn A?
Said differently, can we wrap AtomicApply() with a helper function called AtomicApplyDeadline() (or Retry or Timeout) that will retry up to the {deadline,timeout,retry count}? Where possible we should make it convenient for developers to not have to bother themselves with the consequences of concurrent and conflicting operations. This gets sticky and why we should punt this control to the users w/ a new function interface.
There was a problem hiding this comment.
Right now there's only a single write transaction at the state store level at any given time. If your transaction bounces and won't apply, there are a bunch of cases where you'd need to take specific action before you retry (update an index, for example). For the cases of locks, we already have retry helpers for single locks, so I'd like to avoid adding a retry thing here.
This isn't needed/used yet, but it's a good hook to get in there so we can add more atomic operations in the future. The Go API hides this detail so that feels like a KV-specific API. The implications on the REST API are pretty minimal.
| // replacing them with byte arrays. | ||
| func fixupKVOps(raw interface{}) error { | ||
| // decodeValue decodes the value member of the given operation. | ||
| decodeValue := func(rawKV interface{}) error { |
There was a problem hiding this comment.
Is there a reason these functions are defined locally. There could be some performance gains to having them statically defined and not in a dynamic variable. And this is in somewhat of a hot path.
There was a problem hiding this comment.
Nope - that's a good observation and I'll hoist them out.
| ] | ||
| ``` | ||
|
|
||
| Up to 500 operations may be present in a single transaction. |
There was a problem hiding this comment.
I wonder if we should reduce this to 64 or something initially for sanity to see how people use it. 500 seems like a pretty high limit.
|
LGTM! |
> Transactional Key/Value API: A new /v1/txn API was added that allows > for atomic updates to and fetches from multiple entries in the > key/value store inside of an atomic transaction. This includes > conditional updates based on obtaining locks, and all other key/value > store operations. https://github.com/hashicorp/consul/blob/v0.7.0/CHANGELOG.md hashicorp/consul#2028
Ok this is ready for final review.