Optimize ockam node create #8755
Conversation
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
9 times, most recently
from
357e149 to
bb8b008
Compare
SanjoDeundiak
changed the title
Runtime in a predictable way and reuse it
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
8 times, most recently
from
ff99ca4 to
e53013f
Compare
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
2 times, most recently
from
0e286be to
92b1064
Compare
SanjoDeundiak
changed the title
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
from
92b1064 to
3b24477
Compare
| let cli_state = cli_state.set_tracing_enabled(tracing_configuration.is_enabled()); | ||
|
|
||
| (tracing_configuration, tracing_guard, Some(cli_state)) | ||
| }; |
There was a problem hiding this comment.
what are the implications of not initializing the clistate here? Couldn't we initialize it here in both branches and remove the match cli_state from below?
There was a problem hiding this comment.
The idea is to initialize logging as early as possible. The problem is that if tracing is enabled, we unfortunately need CliState to be able to properly bootstrap it. Therefore, I was able to initialize logging before creating the CliState only if tracing is disabled
| @@ -45,6 +45,7 @@ after_long_help = docs::after_help(AFTER_LONG_HELP) | |||
| pub struct CreateCommand { | |||
| /// Name of the node or a configuration to set up the node. | |||
| /// The configuration can be either a path to a local file or a URL. | |||
| /// TODO: Use Option<String> | |||
There was a problem hiding this comment.
What's the context of this?
There was a problem hiding this comment.
With current approach it's impossible to see if a user has passed "_default_node_name" or no value at all. Then later in the code you can see
if name == DEFAULT_NODE_NAME {
name = random_name();
}
Which is now a hacky way to figure out if the user has provided no value. It would be a surprise for a user who provided "_default_node_name" and then ended up with a random node name, right? Not sure that's a realistic concern in this particular situation (due to low chance someone picking "_default_node_name" as the node name), but using Option would make it a clear distinction and as a consequence - easier to read the code.
| // The default node was stopped, so we can reuse the name | ||
| name = default_node.name(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are these two checks removed, to prevent the clistate calls?
There was a problem hiding this comment.
I think the whole idea of node registry in the sqlite may need some improvements, but I haven't spent much time thinking about it. My main concern is that there are too many operations to achieve simple things, like starting a node. Ideally, creating a background node should not require opening the DB inside the parent process at all. Let's at least make small steps towards that. This check looked like something that doesn't give us much usability, but would slow us down each time we create a node. We need to admit that db calls are not free - they hurt latency, and start rethinking UX based on that. Also, let's maybe attempt replacing node registry and their status by getting information about running processes from the OS. That might turn out both faster and more reliable. WDYT about all of that?
There was a problem hiding this comment.
Also, given default Vault, Identity, etc. creation is already performed by the background node, not the parent process, can we move some of these checks to the background node as well? That should get us to "parent node doesn't need db access" relatively easy. The ideal result would be that parent process only need to perform few things:
- Start the background ockam process
- Print output from that process
- Wait for the completion callback
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
2 times, most recently
from
16d64bf to
c5925c0
Compare
- create tokio `Runtime` in a predictable way and reuse it - rework `Command` usage - add `no_logging` flag to the node - optimize node startup - replace readiness poll with a callback
SanjoDeundiak
force-pushed
the
sanjo/node_callback
branch
from
c5925c0 to
7c2bea9
Compare
No description provided.