bug: write bsos contained within a commit after the batch has been commited. #980
Conversation
The client may sometimes include bsos in the batch commit message. The problem is that due to the way that data is written to spanner, mutations do not retain the ability to see data previously written in the same transaction. This causes collisions. To solve this, treat the bsos included in the commit as another batch update, then commit all the data. This does run the risk of bumping up against the mutation limit, but it ensures the best chance of data consistency. Writing the commit bsos prior to batch commit will result in the "newer" records being overwritten by "older" ones in the batch. Writing the commit bsos after the batch commit runs the same mutation index conflict problem. Closes #882
…mmited. An alternate approach to #882. This approach has sometimes produced index collisions.
src/web/handlers.rs
Outdated
| if !breq.commit { | ||
| if !coll.bsos.valid.is_empty() { |
There was a problem hiding this comment.
nit:
| if !breq.commit { | |
| if !coll.bsos.valid.is_empty() { | |
| if !breq.commit && !coll.bsos.valid.is_empty() { |
There was a problem hiding this comment.
I'm concerned about this. !breq.commit includes both the append call at line 333 , and building the possible response at line 367. Collapsing the if skips the possible early return for requests that do not specify a commit, and run the risk of committing early.
| @@ -416,6 +394,54 @@ pub async fn post_collection_batch( | |||
| return Err(ApiError::from(err).into()); | |||
| }; | |||
|
|
|||
| // write the BSOs contained in the commit request. | |||
| if !coll.bsos.valid.is_empty() { | |||
There was a problem hiding this comment.
should only happen on commit:
| if !coll.bsos.valid.is_empty() { | |
| if commit && !coll.bsos.valid.is_empty() { |
There was a problem hiding this comment.
I agree, that's why I was asking about the change to line 321 which handles the "not commit" messages. That should capture all non-commit messages, no? Meaning that anything that got as far as here should be a commit. I could make things a bit clearer by wrapping this whole section in an else for !breq.commit, but I'm not sure that matches the style we've used before.
…nto bug/882-bso-collision-2
…nto bug/882-bso-collision-2
|
Yes, slightly. A byproduct of that issue generated a number of the sort of conflicts that this issue addresses. |
|
Hmmm. That issue was a feature request that was never merged? Are we talking about the same #822 ? |
…nto bug/882-bso-collision-2
…/syncstorage-rs into bug/882-bso-collision-2
Description
The client may sometimes include bsos in the batch
commit message. The problem is that due to the way
that data is written to spanner, mutations do not
retain the ability to see data previously written
in the same transaction. This causes collisions.
To solve this, treat the bsos included in the commit
as another batch update, then commit all the data.
This does run the risk of bumping up against the
mutation limit, but it ensures the best chance of
data consistency.
Writing the commit bsos prior to batch commit will
result in the "newer" records being overwritten by
"older" ones in the batch.
Writing the commit bsos after the batch commit runs
the same mutation index conflict problem.
Testing
Unit test included
Issue(s)
Closes #882