Draft: Global process registry - resolves #127#194
Draft: Global process registry - resolves #127#194Yoric wants to merge 6 commits intolunatic-solutions:mainfrom
Conversation
|
After discussing this on Discord, I have a clearer idea of the expected API, so here is a v2. Not sure how/where I should write tests. |
withtypes
left a comment
withtypes
left a comment
There was a problem hiding this comment.
Starting to look good! Nice work! I left some remarks and questions.
| ApiError::InvalidData(_) => "invalid_data", | ||
| ApiError::InvalidPathArg(_) => "invalid_path_arg", | ||
| ApiError::InvalidQueryArg(_) => "invalid_query_arg", | ||
| ApiError::ProcessNotFound => "process_not_found", |
There was a problem hiding this comment.
These are more HTTP errors, so we could simply add NotFound(String) and put info "Process not found" in it. It's already obvious from the context what HTTP 404 means. Maybe we don't even need the message, just NotFound
There was a problem hiding this comment.
I'm going to go with no-arg NotFound for the time being.
| get_module: format!("http://{host}/module/{{id}}"), | ||
| add_module: format!("http://{host}/module"), | ||
| get_nodes: format!("http://{host}/nodes"), | ||
| get_process: format!("http://{host}/process/{{name}}"), |
There was a problem hiding this comment.
By the way, we should check how path parms in axum work regarding URL encoding https://en.wikipedia.org/wiki/URL_encoding and also names cannot contain / etc. hmm
There was a problem hiding this comment.
I think that it can contain / as long as it's properly url-encoded.
I'll try and write a test for that.
| control::{api::*, cert::TEST_ROOT_CERT}, | ||
| NodeInfo, | ||
| }; | ||
| use lunatic_process::{ProcessName, ProcessRecord}; |
There was a problem hiding this comment.
Hmm not sure we should require lunatic-process crate just to get a string and node/process id. I think it's ok for the API to simply use String and define it's own {node_id, process_id} struct.
There was a problem hiding this comment.
Actually, we have merged submilisecond impl of ctrl srv which has lunatic-control crate with only types. We could add those types there...
|
|
||
| [dependencies] | ||
| lunatic-common-api = { workspace = true } | ||
| lunatic-control-axum = { workspace = true } |
There was a problem hiding this comment.
No, no, if you need some types that are used both in control axum and here, now you have lunatic-control which should keep types in-common.
There was a problem hiding this comment.
The reason for this dependency is so that I can use ApiError. Should I move ApiError to lunatic-control? If so, I suspect that I will need to introduce from lunatic-control to axum, is that alright?
There was a problem hiding this comment.
I think you don't need them. ApiError is an internal thing, the external interface is HTTP. In the HTTP client you can just match code strings and HTTP statues which are meaningful to handle. Otherwise, you just report error.
| copy_lookup_nodes_results, | ||
| )?; | ||
|
|
||
| linker.func_wrap4_async("lunatic::distributed", "get", get_process)?; |
There was a problem hiding this comment.
Let's put them then in namespace lunatic::distributed::registry
| .or_trap("lunatic::distributed::get")?; | ||
| let name = std::str::from_utf8(name).or_trap("lunatic::distributed::get")?; | ||
|
|
||
| // Sanity check |
3 participants
Well, here is a first draft. I'm fairly confident that I completely misunderstood the task at hand, but this will serve as a basis for further conversations :)