-
Notifications
You must be signed in to change notification settings - Fork 834
Support atomic writes in the docstore#3500
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@eliben @vangent We are trying to add the transactions support/atomic writes in the docstore and this is the high level idea of how can we support that in go-cloud. https://docs.google.com/document/d/1UVj1kmwDfrs5qm8r7X1p4fAFmsdPEeBHcvWJ8zWF1dY/edit?tab=t.0 The PR is just at a high level to support the idea. |
e452b85 to
a3911cb
Compare
a3911cb to
541d19a
Compare
bcf60ed to
04a45a7
Compare
04a45a7 to
c95f790
Compare
| if len(actions) == 0 { | ||
| return | ||
| } | ||
| setErr := func(err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that if anything in this function fails, everything fails (since it's atomic). Document that here.
docstore/docstore.go
Outdated
| return "[" + strings.Join(as, ", ") + "]" | ||
| } | ||
|
|
||
| // AtomicWrites causes all following writes in the list to execute atomically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to execute as a single atomic operation"
(to avoid confusion with the fact that each write already happens atomically by itself)
| @@ -79,7 +79,7 @@ func TestGroupActions(t *testing.T) { | |||
| }{ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add various cases involving atomic writes, mixed with other writes and gets before, after, and concurrently.
d87f80e to
aa357b9
Compare
aa357b9 to
c8a11bd
Compare
|
@jba thanks for your feedback, I addressed all of the comments, please take another look |
e46f0b9 to
9c979be
Compare
|
@jba, I addressed your latest 2 comments |
9c979be to
6a57b2f
Compare
6a57b2f to
8d0616d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3500 +/- ## ========================================== + Coverage 73.26% 73.28% +0.01% ========================================== Files 113 113 Lines 15065 15080 +15 ========================================== + Hits 11038 11051 +13 - Misses 3261 3263 +2 Partials 766 766 View full report in Codecov by Sentry. |
|
@jba the codecov report is under-reporting because the conformance tests are disabled for now but I verified the tests we added are good for dynamo. I will enable the 2 conformance tests added along with the other providers in the follow up PR. |
|
Thanks for the PR! Looking forward to the follow-ups. |