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

feat: ADR-040: Implement DBConnection.Revert #10308

Merged
merged 7 commits into from
Oct 8, 2021

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Oct 5, 2021

Implements the DBConnection.Revert method which reverts DB state to the last saved version. This will be need to implement atomic commits with the KV store for #9892 (supports ADR-040).

Closes: #10308


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@roysc roysc changed the title memdb, badger: implement DBConnection.Revert feat: implement DBConnection.Revert Oct 5, 2021
@roysc roysc changed the title feat: implement DBConnection.Revert feat: ADR-040: Implement DBConnection.Revert Oct 5, 2021
@roysc
Copy link
Contributor Author

roysc commented Oct 5, 2021

This adds implementations for BadgerDB and MemDB; Rocks has the method merged already. Note - this commit
was added after approval and automatically merged without a full review. Please review these changes as well, and I can add any patches in this PR.

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #10308 (7352f47) into master (33c8314) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 7352f47 differs from pull request most recent head e8ebd8a. Consider uploading reports for the commit e8ebd8a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10308      +/-   ##
==========================================
+ Coverage   64.15%   64.18%   +0.02%     
==========================================
  Files         572      572              
  Lines       53983    53983              
==========================================
+ Hits        34635    34651      +16     
+ Misses      17373    17355      -18     
- Partials     1975     1977       +2     
Impacted Files Coverage Δ
snapshots/store.go 71.66% <0.00%> (-1.25%) ⬇️
crypto/keys/internal/ecdsa/privkey.go 84.21% <0.00%> (+1.75%) ⬆️
x/distribution/simulation/operations.go 90.27% <0.00%> (+9.72%) ⬆️

db/badgerdb/db.go Outdated Show resolved Hide resolved
db/badgerdb/db.go Outdated Show resolved Hide resolved
db/badgerdb/db.go Show resolved Hide resolved
db/types.go Outdated Show resolved Hide resolved
@tac0turtle
Copy link
Member

does this coincide with #10281

@roysc
Copy link
Contributor Author

roysc commented Oct 7, 2021

does this coincide with #10281

@marbar3778 serendipity, I think. I wasn't aware of that issue, but it certainly seems like this could be used to implement that rollback.

@robert-zaremba
Copy link
Collaborator

Adding closes statement for #10308

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

utACK

@@ -122,14 +127,17 @@ func readVersionsFile(path string) (*versionManager, error) {
if err != nil {
return nil, err
}
if version == 0 { // 0 maps to the latest TS
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's "TS"? Shall we change this to: "0 maps to the latest version"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timestamp, which is how Badger refers to versions. I want to distinguish between our versions and the Badger timestamps they map to, but I can use the actual word

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, please, let's don't use TS, time or timestamp is better.

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 8, 2021
@mergify mergify bot merged commit 88f39ad into cosmos:master Oct 8, 2021
@roysc roysc deleted the roysc/adr-040-db-revert branch October 8, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants