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

Update Version Checks #18

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Update Version Checks #18

merged 1 commit into from
Jan 25, 2022

Conversation

bbengfort
Copy link
Contributor

@bbengfort bbengfort commented Jan 16, 2022

This PR implements a required change for Trtl Anti-Entropy: Update checks if the version being updated is still later than the local version. I've also added a WithForce() option that bypasses checks if the user wants to force put an object to the database, overriding the namespace and version checks.

This is related to trisacrypto/directory#299

Note this is related to sc-2703 but is not a complete implementation; this PR only does enough to fix sc-2694.

This PR implements a required change for Trtl Anti-Entropy: Update
checks if the version being updated is still later than the local
version. I've also added a WithForce() option that bypasses checks if
the user wants to force put an object to the database, overriding the
namespace and version checks.
@codecov
Copy link

codecov bot commented Jan 16, 2022

Codecov Report

Merging #18 (8f39d82) into main (cc53387) will decrease coverage by 0.07%.
The diff coverage is 61.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
- Coverage   55.08%   55.00%   -0.08%     
==========================================
  Files          10       10              
  Lines         492      509      +17     
==========================================
+ Hits          271      280       +9     
- Misses        178      184       +6     
- Partials       43       45       +2     
Impacted Files Coverage Δ
options/options.go 77.77% <0.00%> (-13.53%) ⬇️
honu.go 48.20% <76.47%> (+2.16%) ⬆️

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 cc53387...8f39d82. Read the comment docs.

Copy link
Member

@rebeccabilbro rebeccabilbro left a comment

Choose a reason for hiding this comment

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

I know we haven't gone through sc-2694 yet @bbengfort @pdeziel, but I think this makes sense to me. I gather that part of the replication bug we've been experiencing in trtl relates to the need for this subsequent check that object updates sent by the remote are still the latest from the perspective of the local replica? And it looks like you've added the force option so that we can still make manual repairs to an object, even if it means regressing its version?

@bbengfort
Copy link
Contributor Author

I know we haven't gone through sc-2694 yet @bbengfort @pdeziel, but I think this makes sense to me. I gather that part of the replication bug we've been experiencing in trtl relates to the need for this subsequent check that object updates sent by the remote are still the latest from the perspective of the local replica? And it looks like you've added the force option so that we can still make manual repairs to an object, even if it means regressing its version?

Yes, this is related to the need to handle Put and Update concurrency correctly while CHECK messages are being sent between replicas. Adding guards like this makes me a little worried sometimes though because it means if things get out of whack, it's tougher to repair - so I'm hoping the force option allows us to do dangerous things when we just need the database to get back to a correct state.

Based on your review of SC-2694, I think we should add one more thing to Update - a return of the UpdateType, e.g. was it a stomp, a skip, a linear version, etc. I'd like to open that in a new PR so that it can be added to SC-2694 to unblock the prometheus metrics.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #2694: Refactor Phase 1/Phase 2 Anti-Entropy and Gossip.

@bbengfort bbengfort merged commit 0d24e25 into main Jan 25, 2022
@bbengfort bbengfort deleted the update-version-check branch January 25, 2022 15:29
@rebeccabilbro
Copy link
Member

Based on your review of SC-2694, I think we should add one more thing to Update - a return of the UpdateType, e.g. was it a stomp, a skip, a linear version, etc. I'd like to open that in a new PR so that it can be added to SC-2694 to unblock the prometheus metrics.

That would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants