feat: Make get_state_dir the overall directory containing all states by ninegua · Pull Request #468 · dfinity/sdk

ninegua · 2020-03-19T00:54:42Z

The recent dfinity replica has gained support for persisting consensus artifacts (dfinity-lab/dfinity#2931). So it has to be configured too. Also there was a crypto_root that didn't get configured for dfx, so it always ended up in /tmp/ic_crypto.

This PR tries to put all three paths as sub-directories under the same env.get_state_dir(). I'm actually not sure if this is correct. Please feel free to suggest otherwise.

…execution state, consensus pool, and crypto.

eftychis · 2020-03-19T00:55:55Z

looking.

@ninegua I am adding feat: on the title.

eftychis · 2020-03-19T00:57:38Z

src/dfx/src/lib/replica_config.rs


 impl ReplicaConfig {
-    pub fn new(state_root: &Path) -> Self {
+    pub fn new(state_root: &PathBuf) -> Self {


? Why would you take a reference to an owned object? Just do as_ref() when passing to ReplicaConfig::new()

How about just passing PathBuf?

Trying to recall where we use state_root. Generally https://doc.rust-lang.org/std/path/index.html gives an idea of how to think. I think we are using join, and we don't create our own pathbuf otherwise, so moving PathBuf in doesn't give us much, while we want state_root around. I think keeping &Path makes a bit more sense here.

The code is actually a bit simpler with PathBuf than with Path, mostly due to other functions are using PathBuf too.

I guess it is fine to move. I am not sure it is simpler. If there is an issue, we can change it again I guess.

eftychis · 2020-03-19T01:00:13Z

src/dfx/src/lib/replica_config.rs

            },
            state_manager: StateManagerConfig {
-                state_root: state_root.to_path_buf(),
+                state_root: state_root.join("ic_state"),


Ok, hm I think cleaning should still work, as we simply remove the directory with all its contents. Can you test it `dfx start --clean). I find ic_state, confusing then honestly. It should be something like execution_state? Or replicated_state (which is what replica uses.)

eftychis

@ninegua Are you going to open the PR?? 😹

eftychis · 2020-03-19T19:32:29Z

oh wait! This is not supported right now right?

eftychis

We need to update dfinity to test this.

ninegua · 2020-03-19T19:43:28Z

I've tested it with latest replica version, and it works correctly. dfx start --clean also works.

eftychis · 2020-03-19T19:49:35Z

The thing is we need to bring in the latest replica version. We should cut a replica release though perhaps. cc @p-shahi

ninegua · 2020-03-19T19:52:16Z

I assume that passing CI means it works with the current pinned version of replica too. So just sticking to sdk's normal release schedule is fine. (that is, take your time with updating replica version)

hansl

It works because the new fields in the TOML are going to be ignored. I'm fine with this PR as is. LGTM.

## Changelog for common: Branch: master Commits: [dfinity-lab/common@36ace2b0...caa21e43](https://github.com/dfinity-lab/common/compare/36ace2b0fc6ad57391c76c09fe5508634f0b1e1d...caa21e43f85b077ed2f4c41feaa0af8ad7d94189) * [`caa21e43`](https://github.com/dfinity-lab/common/commit/caa21e43f85b077ed2f4c41feaa0af8ad7d94189) Removing dot ([dfinity-lab/common⁠#468](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/468))

Make get_state_dir the overall directory containing all states of of …

1d780dd

…execution state, consensus pool, and crypto.

eftychis changed the title ~~Make get_state_dir the overall directory containing all states~~ feat: Make get_state_dir the overall directory containing all states Mar 19, 2020

eftychis reviewed Mar 19, 2020

View reviewed changes

ninegua added 2 commits March 18, 2020 18:06

Fix PathBuf

7c7cab1

Better subdir name

e2f063e

ninegua force-pushed the paulliu/add-crypto-artifact-pool-config branch from d2eebbb to e2f063e Compare March 19, 2020 01:06

eftychis approved these changes Mar 19, 2020

View reviewed changes

eftychis reviewed Mar 19, 2020

View reviewed changes

ninegua marked this pull request as ready for review March 19, 2020 19:42

ninegua requested a review from a team as a code owner March 19, 2020 19:42

hansl approved these changes Mar 20, 2020

View reviewed changes

hansl merged commit 4f98a0a into master Mar 20, 2020

hansl deleted the paulliu/add-crypto-artifact-pool-config branch March 20, 2020 02:42

Comments

Conversation

ninegua commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eftychis commented Mar 19, 2020

Uh oh!

eftychis Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

ninegua Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

eftychis Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninegua Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

eftychis Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

eftychis Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

eftychis left a comment

Choose a reason for hiding this comment

Uh oh!

eftychis commented Mar 19, 2020

Uh oh!

eftychis left a comment

Choose a reason for hiding this comment

Uh oh!

ninegua commented Mar 19, 2020

Uh oh!

eftychis commented Mar 19, 2020

Uh oh!

ninegua commented Mar 19, 2020

Uh oh!

hansl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ninegua commented Mar 19, 2020 •

edited

Loading

eftychis Mar 19, 2020 •

edited

Loading