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

Implement GetDiff E2E tests #1975

Merged
merged 4 commits into from
Aug 23, 2019
Merged

Implement GetDiff E2E tests #1975

merged 4 commits into from
Aug 23, 2019

Conversation

jberci
Copy link
Contributor

@jberci jberci commented Aug 2, 2019 •
edited
Loading

This PR:

  • adds a debug command that checks if it can get all write logs up to the current round,
  • fixes some synchronization bugs in the storage worker,
  • adds E2E tests for storage syncing.

Closes #1932

@kostko kostko changed the base branch from master to kostko/feature/storage-gc August 2, 2019 14:31
kostko
kostko reviewed Aug 2, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Add tests for write log merging.

go/storage/mkvs/urkel/db/badger/badger.go Show resolved Hide resolved
go/storage/mkvs/urkel/db/leveldb/leveldb.go Show resolved Hide resolved
go/storage/mkvs/urkel/db/badger/badger.go Outdated Show resolved Hide resolved
go/storage/mkvs/urkel/db/badger/badger.go Outdated Show resolved Hide resolved
go/storage/mkvs/urkel/db/leveldb/leveldb.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/storage-gc branch 2 times, most recently from 36c8e10 to 70001b9 Compare August 6, 2019 09:43
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from f544878 to e57ea55 Compare August 7, 2019 00:49
kostko
kostko reviewed Aug 7, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

I would split the commits into three parts:

  • Add write log merging.
  • Improve syncing (tracking outstanding roots, retries).
  • Add E2E tests.

Otherwise only minor nits and cleaning up the debug statements. The new syncing code and tests look good.

.buildkite/scripts/test_e2e.sh Outdated Show resolved Hide resolved
.buildkite/scripts/test_e2e.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go/ekiden/cmd/debug/storage/storage.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/committee/node.go Show resolved Hide resolved
go/worker/storage/committee/node.go Outdated Show resolved Hide resolved
go/worker/storage/worker.go Outdated Show resolved Hide resolved
go/worker/storage/worker.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/storage-gc branch from 70001b9 to ca24117 Compare August 8, 2019 16:44
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from 0db4eaf to d439ff3 Compare August 9, 2019 15:48
@codecov
Copy link

codecov bot commented Aug 9, 2019 •
edited
Loading

Codecov Report

Merging #1975 into master will decrease coverage by 0.19%.
The diff coverage is 36.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1975     +/-   ##
=========================================
- Coverage   55.97%   55.77%   -0.2%     
=========================================
  Files         247      249      +2     
  Lines       25089    25344    +255     
=========================================
+ Hits        14043    14136     +93     
- Misses       9534     9685    +151     
- Partials     1512     1523     +11
Impacted Files Coverage Δ
go/storage/init.go 59.61% <0%> (-0.77%) ⬇️
go/ekiden/cmd/debug/roothash/roothash.go 6.02% <0%> (ø) ⬆️
go/ekiden/cmd/node/node.go 51.68% <100%> (+0.14%) ⬆️
go/storage/client/init.go 78.68% <100%> (ø) ⬆️
go/ekiden/cmd/debug/debug.go 100% <100%> (ø) ⬆️
go/worker/storage/worker.go 66.66% <100%> (+1.09%) ⬆️
go/storage/client/client.go 63.52% <100%> (ø) ⬆️
go/storage/client/watcher.go 77.84% <100%> (+0.25%) ⬆️
go/storage/client/tests/tests.go 92.45% <100%> (ø) ⬆️
go/worker/storage/grpc.go 17.64% <17.64%> (ø)
... and 8 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 870550d...e19939f. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/storage-gc branch 3 times, most recently from b6492a5 to 7b9d9a5 Compare August 12, 2019 12:26
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from d439ff3 to 7f46195 Compare August 12, 2019 15:22
@jberci jberci changed the title Jberci/feature/e2e getdiff Implement GetDiff E2E tests Aug 12, 2019
@jberci jberci marked this pull request as ready for review August 12, 2019 15:46
kostko
kostko approved these changes Aug 12, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

I think this looks good except for the minor nits. Feel free to squash/reorganize commits.

You should just wait before merging this so that #1902 is merged into master first and the base of this PR is changed to be master.

go/storage/grpc.go Outdated Show resolved Hide resolved
go/worker/storage/worker.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/storage-gc branch 3 times, most recently from cc89e26 to 1e5e0be Compare August 13, 2019 08:25
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch 2 times, most recently from 018dccf to d79662c Compare August 13, 2019 13:44
@kostko kostko force-pushed the kostko/feature/storage-gc branch from 61ba378 to 25afa74 Compare August 13, 2019 16:45
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from d79662c to 971e5a5 Compare August 14, 2019 17:07
@kostko kostko force-pushed the kostko/feature/storage-gc branch 4 times, most recently from d2e3c79 to ec92630 Compare August 20, 2019 16:18
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from 971e5a5 to 701afb4 Compare August 21, 2019 11:59
@kostko kostko force-pushed the kostko/feature/storage-gc branch from ec92630 to 9f50be2 Compare August 21, 2019 12:12
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from 701afb4 to 6227e26 Compare August 21, 2019 15:33
@jberci jberci changed the base branch from kostko/feature/storage-gc to master August 21, 2019 15:33
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch 3 times, most recently from 5e43056 to 1e6e48c Compare August 22, 2019 14:57
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from d092c79 to 5a62dec Compare August 22, 2019 17:09
kostko
kostko approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Went over the last commit, looks good except the minor nit.

go/storage/client/watcher.go Show resolved Hide resolved
@jberci jberci force-pushed the jberci/feature/e2e-getdiff branch from 5a62dec to e19939f Compare August 23, 2019 12:09
@jberci jberci merged commit f3b62ad into master Aug 23, 2019
@jberci jberci deleted the jberci/feature/e2e-getdiff branch August 23, 2019 12:43
@kostko kostko mentioned this pull request Aug 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostko kostko kostko approved these changes

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

@Yawning Yawning Awaiting requested review from Yawning

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

Successfully merging this pull request may close these issues.

Storage sync E2E tests
2 participants
@jberci @kostko