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

Delete leaves from queue in UpdateLeaves #2159

Merged
merged 4 commits into from
Jun 25, 2020
Merged

Conversation

gdbelvin
Copy link
Contributor

This change brings the mysql implementation into line with what spanner does so we can have a consistent behavior between implementations.

Previous behavior:

  • mysql impl deleted leaves from the queue inside Dequeue
  • spanner does not delete inside Dequeue, it does it in UpdateLeaves

New / spanner behavior:

@gdbelvin gdbelvin requested a review from a team as a code owner June 23, 2020 07:54
@gdbelvin gdbelvin requested a review from AlCutter June 23, 2020 07:54
@AlCutter AlCutter requested a review from Martin2112 June 23, 2020 09:13
@Martin2112
Copy link
Contributor

Looks like the batched queue version of the code will need to be adjusted as well.

@gdbelvin
Copy link
Contributor Author

After taking a closer look at the Batched Queue implementation, I'm not sure if I can make this change. The batched queue version takes QueueID, a value not passed in LogLeaf.

I'm unsure how to proceed now, since there's not really a behavior we can standardize and test against between the different storage implementations.

@gdbelvin
Copy link
Contributor Author

gdbelvin commented Jun 23, 2020

@Martin2112 @AlCutter -- would love your thoughts on this. I was going to abandon this, but the cloudspanner dequeue operations are currently broken, and this is a prerequisite to having dequeue tests.

Update - I think I have a solution

@gdbelvin gdbelvin mentioned this pull request Jun 23, 2020
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #2159 into master will increase coverage by 0.21%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2159      +/-   ##
==========================================
+ Coverage   64.24%   64.46%   +0.21%     
==========================================
  Files         116      116              
  Lines       10002     9919      -83     
==========================================
- Hits         6426     6394      -32     
+ Misses       2958     2921      -37     
+ Partials      618      604      -14     
Impacted Files Coverage Δ
storage/mysql/queue.go 67.24% <75.00%> (+24.10%) ⬆️
storage/mysql/log_storage.go 68.22% <77.77%> (-0.55%) ⬇️
merkle/log_proofs.go 93.93% <0.00%> (+24.65%) ⬆️

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 67ba297...0deeef5. Read the comment docs.

@gdbelvin
Copy link
Contributor Author

@Martin2112 the build is passing now. Not quite sure why the github / travis hook isn't picking it up.

storage/mysql/queue.go Show resolved Hide resolved
storage/mysql/queue.go Outdated Show resolved Hide resolved
storage/mysql/log_storage.go Show resolved Hide resolved
@gdbelvin gdbelvin requested a review from Martin2112 June 24, 2020 16:50
@gdbelvin
Copy link
Contributor Author

Tweeked the error message. Anything else needed?

@Martin2112
Copy link
Contributor

As long as we're happy that this is being properly tested and won't cause issues for users.

@gdbelvin gdbelvin merged commit 6c256bd into google:master Jun 25, 2020
@gdbelvin gdbelvin deleted the fixmysql2 branch June 25, 2020 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants