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

Draft: Global process registry - resolves #127 #194

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Yoric
Copy link

@Yoric Yoric commented Feb 28, 2023

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 :)

@withtypes withtypes marked this pull request as draft March 2, 2023 19:07
@Yoric Yoric marked this pull request as ready for review March 13, 2023 08:49
@Yoric
Copy link
Author

Yoric commented Mar 13, 2023

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.

Copy link
Contributor

@withtypes withtypes left a comment

Choose a reason for hiding this comment

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

Starting to look good! Nice work! I left some remarks and questions.

@@ -42,6 +43,7 @@ impl ApiError {
ApiError::InvalidData(_) => "invalid_data",
ApiError::InvalidPathArg(_) => "invalid_path_arg",
ApiError::InvalidQueryArg(_) => "invalid_query_arg",
ApiError::ProcessNotFound => "process_not_found",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to go with no-arg NotFound for the time being.

@@ -50,6 +51,9 @@ pub async fn register(
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}}"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I think that it can contain / as long as it's properly url-encoded.

I'll try and write a test for that.

crates/lunatic-control-axum/src/routes.rs Show resolved Hide resolved
Extension, Json, Router,
};
use lunatic_distributed::{
control::{api::*, cert::TEST_ROOT_CERT},
NodeInfo,
};
use lunatic_process::{ProcessName, ProcessRecord};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we have merged submilisecond impl of ctrl srv which has lunatic-control crate with only types. We could add those types there...

@@ -9,6 +9,7 @@ license = "Apache-2.0/MIT"

[dependencies]
lunatic-common-api = { workspace = true }
lunatic-control-axum = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -43,6 +43,11 @@ where
"copy_lookup_nodes_results",
copy_lookup_nodes_results,
)?;

linker.func_wrap4_async("lunatic::distributed", "get", get_process)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants