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

Make storage per-runtime #2494

Merged
merged 5 commits into from
Dec 28, 2019
Merged

Make storage per-runtime #2494

merged 5 commits into from
Dec 28, 2019

Conversation

kostko
Copy link
Member

@kostko kostko commented Dec 20, 2019

Fixes #2474

  • First commit removes the global node storage backend and instead replaces it with per-runtime storage backends. To support answering queries for multiple runtimes via gRPC a runtime registry-based storage router is added.

This PR changes database file organization within the node data directory and storage database internals and as such it is BREAKING WITHOUT MIGRATION.

TODO

  • Remove namespacing within a single storage database, add a namespace argument to storage backends for cross-checks.
  • Client storage backend should automatically track the correct runtime. Also consider removing support for tracking multiple runtimes which increases complexity and should now no longer be needed (as there is a storage instance per runtime).
  • Make untrusted local storage per runtime.

@kostko kostko added the c:breaking/consensus Category: breaking consensus changes label Dec 20, 2019
@kostko kostko force-pushed the kostko/feature/storage-per-rt branch 2 times, most recently from bb03e71 to a8ba992 Compare December 20, 2019 13:23
@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #2494 into master will increase coverage by <.01%.
The diff coverage is 73.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2494      +/-   ##
==========================================
+ Coverage   66.86%   66.86%   +<.01%     
==========================================
  Files         324      325       +1     
  Lines       29894    29937      +43     
==========================================
+ Hits        19988    20018      +30     
- Misses       7413     7415       +2     
- Partials     2493     2504      +11
Impacted Files Coverage Δ
go/storage/mkvs/urkel/db/api/api.go 71.05% <ø> (ø) ⬆️
go/worker/common/worker.go 88.05% <ø> (+3.6%) ⬆️
go/storage/metrics.go 70.53% <ø> (-1.02%) ⬇️
go/runtime/history/db.go 81.14% <ø> (ø) ⬆️
.../storage/mkvs/urkel/interop/cmd/protocol_server.go 13.2% <0%> (-0.26%) ⬇️
go/oasis-node/cmd/storage/benchmark/benchmark.go 3.92% <0%> (ø) ⬆️
go/runtime/client/client.go 69.35% <100%> (ø) ⬆️
go/worker/storage/committee/node.go 74.86% <100%> (+1.01%) ⬆️
go/oasis-node/cmd/node/unsafe_reset.go 63.63% <100%> (ø) ⬆️
go/runtime/history/history.go 77.08% <100%> (ø) ⬆️
... and 30 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 609047e...067af88. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/storage-per-rt branch 3 times, most recently from 98e8b68 to 6456f08 Compare December 23, 2019 12:14
@kostko kostko requested review from ptrus and Yawning December 23, 2019 12:15
@kostko kostko force-pushed the kostko/feature/storage-per-rt branch from 6456f08 to 4ea72c1 Compare December 23, 2019 13:08
@kostko kostko marked this pull request as ready for review December 23, 2019 13:17
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.

code seems fine. but I want to know the background in #2474

go/roothash/tests/tester.go Show resolved Hide resolved
@kostko
Copy link
Member Author

kostko commented Dec 24, 2019

but I want to know the background in #2474

As mentioned on Slack (and in the changelog fragment), the main point for doing this is to simplify the storage database schema, so per-key namespacing is no longer required. It also makes it simpler to remove or copy data for a specific runtime.

@kostko kostko force-pushed the kostko/feature/storage-per-rt branch 2 times, most recently from 24fc863 to 23cbfee Compare December 24, 2019 12:43
This commit removes the global node storage backend and instead replaces
it with per-runtime storage backends. To support answering queries for
multiple runtimes via gRPC a runtime registry-based storage router is
added.
Since the expected usage is now to create a storage client for each
runtime (namespace), the client can be simplified to only support
tracking a single runtime.
@kostko kostko force-pushed the kostko/feature/storage-per-rt branch from 23cbfee to dbb0c72 Compare December 26, 2019 20:13
@Yawning
Copy link
Contributor

Yawning commented Dec 27, 2019

Do we want to reduce the storage cache sizes (or make it configurable)?

@kostko
Copy link
Member Author

kostko commented Dec 27, 2019

Good idea, I'll make it configurable.

Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

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

I'm ok with this once the cache size changes go in.

@kostko kostko force-pushed the kostko/feature/storage-per-rt branch 2 times, most recently from 7c3866a to 90b1f32 Compare December 28, 2019 14:26
@kostko kostko force-pushed the kostko/feature/storage-per-rt branch from 90b1f32 to 067af88 Compare December 28, 2019 14:50
@kostko kostko merged commit 95370a2 into master Dec 28, 2019
@kostko kostko deleted the kostko/feature/storage-per-rt branch December 28, 2019 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/consensus Category: breaking consensus changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make storage database per-runtime
4 participants