Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docstore: fix batch Creates #2726

Merged
merged 3 commits into from Jan 28, 2020
Merged

docstore: fix batch Creates #2726

merged 3 commits into from Jan 28, 2020

Conversation

vangent
Copy link
Contributor

@vangent vangent commented Jan 17, 2020

Fixes #2725.

As described in the bug, a batched set of actions with multiple Create actions would drop all but one of the Creates. This is because an internal grouping operation used to order actions so that Gets and Writes for the same key are in an appropriate order uses Key as a map key. Create actions may have a nil Key, so only one survived.

To fix this, keep a separate slice of actions with nil Keys during the grouping, and append them to the "write" group at the end.

@vangent vangent requested a review from jba January 17, 2020 04:31
@googlebot googlebot added the cla: yes Google CLA has been signed! label Jan 17, 2020
@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #2726 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2726      +/-   ##
==========================================
- Coverage   68.28%   68.23%   -0.06%     
==========================================
  Files         115      115              
  Lines       13273    13273              
==========================================
- Hits         9064     9057       -7     
- Misses       3548     3554       +6     
- Partials      661      662       +1
Impacted Files Coverage Δ
internal/retry/retry.go 88.23% <0%> (-11.77%) ⬇️
docstore/docstore.go 87.76% <0%> (-1.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25bbf62...dad626d. Read the comment docs.

for _, a := range actions {
if a.Kind == Get {
if a.Key == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Key is not necessarily nil, it could be empty string. Probably better check a.Kind == Create

@vangent
Copy link
Contributor Author

vangent commented Jan 25, 2020

@eliben or @jba ping?

@vangent vangent merged commit 6b3f548 into google:master Jan 28, 2020
@vangent vangent deleted the docstorecreate branch January 28, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docstore/driver: batch Create would only create single document across all drivers
5 participants