Conversation
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
KeranYang
changed the title
KeranYang
changed the title
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3180 +/- ##
==========================================
+ Coverage 80.31% 80.34% +0.03%
==========================================
Files 297 300 +3
Lines 67640 68028 +388
==========================================
+ Hits 54323 54660 +337
- Misses 12764 12818 +54
+ Partials 553 550 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KeranYang
changed the title
@whynowy to answer your question on my previous PR, yes, this PR achieves starting both gRPC and HTTP servers. |
|
Since this is being merged into master, can we modularize the code and ensure it’s properly unit tested? I’m fine with merging if it’s well tested and serves a specific purpose, but in its current form this feels more like a POC. |
rust/numaflow-daemon/src/lib.rs
Outdated
| let (http_tx, mut http_rx) = mpsc::channel(1000); | ||
|
|
||
| // Start a thread to accept requests. | ||
| let _accept_req_task = tokio::spawn(async move { |
There was a problem hiding this comment.
Can we make sure tokio tasks exit? spawned tasks are not tracked
rust/numaflow-daemon/src/lib.rs
Outdated
| let grpc_res = grpc_server_task.await?; | ||
| grpc_res?; | ||
|
|
||
| // TODO - Gracefully shutdown. |
There was a problem hiding this comment.
graceful shutdown is critical, can we tackle that before merging the PR?
rust/numaflow-daemon/src/lib.rs
Outdated
| // Send to the HTTP channel by default. | ||
| // This is because most of the time, HTTP is used for communication. | ||
| // On Numaflow, if a client is sending a gRPC request, the h2 protocol is explicitly used. | ||
| let _ = http_sender.send(stream).await; |
|
|
||
| let addr = format!("[::]:{}", DAEMON_SERVICE_PORT).parse()?; | ||
| // Create a TCP listener that can listen to both h2 and http 1.1. | ||
| let addr: SocketAddr = format!("[::]:{}", DAEMON_SERVICE_PORT).parse()?; |
There was a problem hiding this comment.
Can we split this more functions like start_grpc_server(),
start_http_server(), serve_requests()? to make it more readable also makes it easy to review the code.
|
@yhl25 Thank you for the detailed review. I will address them as part of this PR. FYI, I am re-writing daemon in rust, one small PR at a time and till it achieves feature parity with golang, it remains dead code. See: |
Dead code is fine, since its being merged to master please make sure the code is production quality. |
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
4 participants
What this PR does / why we need it
Rust MonoVertex server can now serve both gRPC and HTTP requests.
Testing
Tested by running rust daemon server and successfully verified the /readyz and /livez endpoints (backed by HTTP/1.1) worked.
Special notes for reviewers
It continues to be dead code, meaning merging this PR won't affect the existing daemon server backed by GoLang.