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.

Sign up for GitHub

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

Jump to bottom

Various MKVS remote syncer bug fixes #2775

Merged
merged 6 commits into from
Mar 25, 2020
Merged

Various MKVS remote syncer bug fixes #2775

merged 6 commits into from
Mar 25, 2020

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 23, 2020

No description provided.

@codecov
Copy link

codecov bot commented Mar 23, 2020 •
edited

Codecov Report

Merging #2775 into master will decrease coverage by 0.23%.
The diff coverage is 68.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2775      +/-   ##
==========================================
- Coverage   63.12%   62.89%   -0.24%     
==========================================
  Files         389      389              
  Lines       37047    37062      +15     
==========================================
- Hits        23385    23309      -76     
- Misses      10703    10790      +87     
- Partials     2959     2963       +4
Impacted Files Coverage Δ
go/common/version/version.go 84% <ø> (ø) ⬆️
go/storage/mkvs/urkel/remove.go 85.85% <100%> (+0.44%) ⬆️
go/storage/mkvs/urkel/writelog/writelog.go 25% <100%> (ø) ⬆️
go/storage/mkvs/urkel/urkel.go 82.75% <33.33%> (-6.53%) ⬇️
go/storage/mkvs/urkel/lookup.go 71.42% <60%> (-3.58%) ⬇️
go/storage/mkvs/urkel/cache.go 82.97% <75%> (ø) ⬆️
.../oasis-node/cmd/debug/txsource/workload/runtime.go 1.77% <0%> (-55.63%) ⬇️
go/worker/compute/executor/committee/state.go 74.07% <0%> (-11.12%) ⬇️
go/worker/common/host/protocol/protocol.go 65.67% <0%> (-6.72%) ⬇️
...ompute/txnscheduler/algorithm/batching/batching.go 78.66% <0%> (-6.67%) ⬇️
... and 28 more

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 57b86bb...57cfbf2. Read the comment docs.

@kostko kostko mentioned this pull request Mar 23, 2020
Merged
4 tasks
@kostko kostko changed the title MKVS bug fixes Various MKVS remote syncer bug fixes Mar 23, 2020
@kostko kostko force-pushed the kostko/fix/mkvs-bug-fixes branch 2 times, most recently from b0e6532 to f54c9aa Compare March 23, 2020 17:38
pro-wh
pro-wh reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

gonna have to delegate some

.changelog/2775.bugfix.2.md Show resolved Hide resolved
go/storage/mkvs/urkel/cache.go Show resolved Hide resolved
go/storage/mkvs/urkel/lookup.go Show resolved Hide resolved
runtime/src/storage/mkvs/urkel/tree/lookup.rs Show resolved Hide resolved
go/storage/mkvs/urkel/testdata/case-5.json Show resolved Hide resolved
go/storage/mkvs/urkel/fuzz/tree.go Show resolved Hide resolved
go/storage/mkvs/urkel/fuzz/tree.go Show resolved Hide resolved
go/storage/mkvs/urkel/fuzz/tree.go Show resolved Hide resolved
go/storage/mkvs/urkel/fuzz/tree.go Show resolved Hide resolved
runtime/src/storage/mkvs/urkel/tree/tree_test.rs Show resolved Hide resolved
@kostko kostko force-pushed the kostko/fix/mkvs-bug-fixes branch 4 times, most recently from 46e4855 to 882c2a1 Compare March 24, 2020 16:02
matevz
matevz previously requested changes Mar 24, 2020
View reviewed changes
.changelog/2775.breaking.md Outdated Show resolved Hide resolved
go/storage/mkvs/urkel/lookup.go Show resolved Hide resolved
pro-wh
pro-wh reviewed Mar 24, 2020
View reviewed changes
.changelog/2775.breaking.md Outdated Show resolved Hide resolved
@kostko
go/storage/mkvs: Persist after MergeVerifiedSubtree is done
099f884
@kostko
go/storage/mkvs: Don't forget to include siblings in SyncGet proof
27e1287
@kostko
storage/mkvs: Don't try to sync dirty keys
aac275d
@kostko
go/storage/mkvs: Add limited fuzzing of remote trees
334b38b
@kostko
storage/mkvs: Only nil value should mean deletion
21a5216 
Previously an empty value in the write log signalled that the given entry is a
delete operation instead of an insert one. This was incorrect as inserting an
empty byte string is allowed. This changes

This change is BREAKING as it changes write log semantics and thus it breaks
the runtime worker-host protocol.
@kostko
runtime/storage/mkvs: Test against Go test vectors
57cfbf2
@kostko kostko requested a review from matevz March 24, 2020 17:24
@kostko kostko dismissed matevz’s stale review March 24, 2020 18:37

Comments addressed.

pro-wh
pro-wh approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

lg thanks for the fixes

@kostko kostko merged commit abef603 into master Mar 25, 2020
@kostko kostko deleted the kostko/fix/mkvs-bug-fixes branch March 25, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abukosek abukosek abukosek left review comments

@pro-wh pro-wh pro-wh approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@Yawning Yawning Awaiting requested review from Yawning

@matevz matevz Awaiting requested review from matevz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kostko @matevz @abukosek @pro-wh