chore: make serving config compatible with numaflow-core configs #2291

BulkBeing · 2024-12-17T05:16:31Z

Closes #2280

Removes files in serving/config/ and moved the values to corresponding struct's Default trait implementation
Removes usage of global static configuration. Instead, parse the config in the starting of the application and pass it around.

Tested by running the pipeline https://github.com/numaproj/numaproj-demo/blob/main/serving-demo/README.md

Signed-off-by: Sreekanth <[email protected]>

codecov · 2024-12-17T05:25:32Z

Codecov Report

Attention: Patch coverage is 81.58348% with 107 lines in your changes missing coverage. Please review.

Project coverage is 65.76%. Comparing base (10b4978) to head (bf0c330).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
rust/numaflow-core/src/config/components.rs	0.00%	36 Missing ⚠️
rust/numaflow/src/main.rs	0.00%	22 Missing ⚠️
rust/serving/src/lib.rs	0.00%	17 Missing ⚠️
rust/serving/src/config.rs	90.30%	16 Missing ⚠️
rust/serving/src/app.rs	92.45%	8 Missing ⚠️
rust/serving/src/app/callback.rs	86.11%	5 Missing ⚠️
rust/serving/src/app/jetstream_proxy.rs	97.33%	2 Missing ⚠️
rust/numaflow-core/src/shared/create_components.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2291      +/-   ##
==========================================
+ Coverage   65.49%   65.76%   +0.27%     
==========================================
  Files         346      346              
  Lines       42394    42767     +373     
==========================================
+ Hits        27764    28127     +363     
- Misses      13560    13562       +2     
- Partials     1070     1078       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vigith · 2024-12-17T05:45:39Z

rust/serving/src/app.rs

-        env::var(ENV_NUMAFLOW_SERVING_JETSTREAM_PASSWORD),
-    ) {
-        (Ok(user), Ok(password)) => {
+    let js_client = match js_config.auth.as_ref() {


this will go away eventually right? we will directly write to ISB via the pipeline framework.

Yes, for now I'm trying to get it to a working state with minimal changes.

Signed-off-by: Sreekanth <[email protected]>

vigith · 2024-12-18T16:16:34Z

rust/numaflow-core/src/config/components.rs

+            if let Some(ttl) = cfg.store.ttl {
+                if ttl.is_negative() {
+                    return Err(Error::Config(format!(
+                        "TTL value for Redis store can not be negative. Provided value = {ttl:?}"


Suggested change

"TTL value for Redis store can not be negative. Provided value = {ttl:?}"

"TTL value for the store can not be negative. Provided value = {ttl:?}"

removed the word Redis because DDB night come too.

vigith · 2024-12-18T16:23:51Z

rust/serving/src/app/jetstream_proxy.rs

 const NUMAFLOW_RESP_ARRAY_LEN: &str = "Numaflow-Array-Len";
 const NUMAFLOW_RESP_ARRAY_IDX_LEN: &str = "Numaflow-Array-Index-Len";

-#[derive(Clone)]
 struct ProxyState<T> {


don't the state need to be Clone anymore? make sure it is a cheap clone if there is a need.

The router now takes Arc<ProxyState> as the state.

vigith · 2024-12-18T16:24:46Z

rust/serving/src/app/jetstream_proxy.rs


    if let Err(e) = publish_to_jetstream(
-        proxy_state.stream,
+        proxy_state.stream.clone(),


is this clone too much for every request? we know that String is a const since it is stream name.

Currently, a lot of clones are happening for Router states. I will work on a separate PR to avoid those. Need to figure out whether to use Arc of the struct or Arc of individual fields, some handlers only need part of the application state. Since config lives for the duration of the application, we could make all strings to &'static str with Box::leak to avoid all cloning or use Arc<str> instead of String for cheaper cloning.

please do as a follow up

vigith · 2024-12-18T16:25:29Z

rust/serving/src/app/jetstream_proxy.rs

    )
    .await
    {
        // Deregister the ID in the callback proxy state if writing to Jetstream fails
-        let _ = proxy_state.callback.deregister(&id).await;
+        let _ = proxy_state.callback.clone().deregister(&id).await;


same for callback.clone

vigith · 2024-12-18T16:27:16Z

rust/serving/src/config.rs

+const ENV_NUMAFLOW_SERVING_HOST_IP: &str = "NUMAFLOW_SERVING_HOST_IP";
+const ENV_NUMAFLOW_SERVING_APP_PORT: &str = "NUMAFLOW_SERVING_APP_LISTEN_PORT";
+const ENV_NUMAFLOW_SERVING_JETSTREAM_USER: &str = "NUMAFLOW_ISBSVC_JETSTREAM_USER";
+const ENV_NUMAFLOW_SERVING_JETSTREAM_PASSWORD: &str = "NUMAFLOW_ISBSVC_JETSTREAM_PASSWORD";
+const ENV_NUMAFLOW_SERVING_AUTH_TOKEN: &str = "NUMAFLOW_SERVING_AUTH_TOKEN";


create a mod called serving and move serving items there? config is getting bloated

This is the serving::config module. As of now, it not that big. I think single module is fine for now.

vigith · 2024-12-18T16:29:23Z

rust/serving/src/config.rs

coverage is getting low

Signed-off-by: Sreekanth <[email protected]>

BulkBeing added 2 commits December 16, 2024 14:58

Remove usage of global static configuration in Serving

fae9816

Signed-off-by: Sreekanth <[email protected]>

Fix unit tests

8673739

Signed-off-by: Sreekanth <[email protected]>

vigith changed the title ~~Make serving config compatible with numaflow-core configs~~ chore: make serving config compatible with numaflow-core configs Dec 17, 2024

vigith reviewed Dec 17, 2024

View reviewed changes

BulkBeing added 3 commits December 17, 2024 11:29

Serving: Remove static config files

e08ddc4

Signed-off-by: Sreekanth <[email protected]>

Use app listen port set using env variable

2248415

Signed-off-by: Sreekanth <[email protected]>

Remove usage of global static config for pipeline sepc, fix unit tests

337dcc3

Signed-off-by: Sreekanth <[email protected]>

BulkBeing marked this pull request as ready for review December 18, 2024 08:10

BulkBeing requested a review from whynowy as a code owner December 18, 2024 08:10

BulkBeing requested review from yhl25 and vigith December 18, 2024 08:17

vigith reviewed Dec 18, 2024

View reviewed changes

BulkBeing added 2 commits December 19, 2024 09:15

Unit tests for config parsing

bdbb23e

Signed-off-by: Sreekanth <[email protected]>

Unit tests for jetstream basic auth

bf0c330

Signed-off-by: Sreekanth <[email protected]>

vigith approved these changes Dec 19, 2024

View reviewed changes

BulkBeing mentioned this pull request Dec 19, 2024

Reduce application state cloning in Serving #2296

Open

yhl25 approved these changes Dec 19, 2024

View reviewed changes

BulkBeing merged commit f41e6ff into main Dec 19, 2024
28 checks passed

BulkBeing deleted the serving-source-config branch December 19, 2024 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make serving config compatible with numaflow-core configs #2291

chore: make serving config compatible with numaflow-core configs #2291

BulkBeing commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

vigith Dec 17, 2024

BulkBeing Dec 17, 2024

vigith Dec 18, 2024

vigith Dec 18, 2024

BulkBeing Dec 19, 2024

vigith Dec 18, 2024

BulkBeing Dec 19, 2024

vigith Dec 19, 2024

BulkBeing Dec 19, 2024

vigith Dec 18, 2024

vigith Dec 18, 2024

BulkBeing Dec 19, 2024

vigith Dec 18, 2024

	"TTL value for Redis store can not be negative. Provided value = {ttl:?}"
	"TTL value for the store can not be negative. Provided value = {ttl:?}"

chore: make serving config compatible with numaflow-core configs #2291

chore: make serving config compatible with numaflow-core configs #2291

Conversation

BulkBeing commented Dec 17, 2024 • edited Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BulkBeing commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading