Skip to content

Intercept during routing #1864

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

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

Intercept during routing #1864

wants to merge 51 commits into from

Conversation

fuzzypixelz
Copy link
Member

@fuzzypixelz fuzzypixelz commented Mar 31, 2025

Depends on #1828.

@fuzzypixelz fuzzypixelz added the enhancement Existing things could work better label Mar 31, 2025
@eclipse-zenoh eclipse-zenoh deleted a comment from github-actions bot Mar 31, 2025
@eclipse-zenoh eclipse-zenoh deleted a comment from github-actions bot Mar 31, 2025
Copy link

codecov bot commented Mar 31, 2025

Codecov Report

Attention: Patch coverage is 60.04326% with 1293 lines in your changes missing coverage. Please review.

Project coverage is 71.15%. Comparing base (83b874e) to head (34014ff).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
zenoh/src/net/routing/hat/router/queries.rs 24.82% 215 Missing ⚠️
zenoh/src/net/routing/hat/linkstate_peer/pubsub.rs 24.18% 163 Missing ⚠️
zenoh/src/net/routing/hat/router/pubsub.rs 42.90% 161 Missing ⚠️
...enoh/src/net/routing/hat/linkstate_peer/queries.rs 27.97% 139 Missing ⚠️
zenoh/src/net/routing/hat/router/token.rs 56.84% 123 Missing ⚠️
zenoh/src/net/routing/hat/linkstate_peer/token.rs 50.54% 90 Missing ⚠️
zenoh/src/net/routing/hat/p2p_peer/queries.rs 59.57% 76 Missing ⚠️
zenoh/src/net/routing/hat/p2p_peer/pubsub.rs 66.21% 74 Missing ⚠️
zenoh/src/net/routing/hat/p2p_peer/token.rs 68.44% 71 Missing ⚠️
zenoh/src/net/routing/hat/client/token.rs 59.85% 55 Missing ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
- Coverage   71.28%   71.15%   -0.14%     
==========================================
  Files         364      364              
  Lines       65664    65605      -59     
==========================================
- Hits        46807    46679     -128     
- Misses      18857    18926      +69     

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

@fuzzypixelz fuzzypixelz force-pushed the intercept-during-routing branch from b565e89 to 1a64c5b Compare April 3, 2025 08:33
@fuzzypixelz fuzzypixelz changed the title Intercept during routing & Pass messages by ref Intercept during routing Apr 3, 2025
@fuzzypixelz fuzzypixelz force-pushed the intercept-during-routing branch 2 times, most recently from 8835564 to 27a4761 Compare April 8, 2025 13:19
@fuzzypixelz fuzzypixelz force-pushed the intercept-during-routing branch from 27a4761 to 68ef1c7 Compare April 8, 2025 13:43
@fuzzypixelz
Copy link
Member Author

Here are is a throughput performance comparison for 8-byte payloads with ACL enabled both on the publisher & the subscriber side between 9ad5304 (main) and 5fae354 (this pull request):

pub-sub-acl3

We gain more than 300k messages per second for the median (as well as the mean) throughput on an Ubuntu Server 24.04 machine with a 12th Gen Intel(R) Core(TM) i5-1240P and 16GB of memory.

@fuzzypixelz fuzzypixelz marked this pull request as ready for review April 14, 2025 16:14
.cloned()
.collect::<Vec<Arc<FaceState>>>()
.collect::<Vec<_>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just .collect_vec() ?

Copy link
Member Author

@fuzzypixelz fuzzypixelz Apr 15, 2025

Choose a reason for hiding this comment

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

I'd like to avoid depending on itertools.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are still being used across zenoh crate (i.e. in low-pass and publisher builder)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. But I don't see the value of saving a few characters at the cost of making it harder to remove itertools down the line...

.cloned()
.collect::<Vec<Arc<FaceState>>>()
.collect::<Vec<_>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just collect_vec() ?

}

pub(crate) fn get_egress_cache(
pub(crate) fn interceptor_cache(
Copy link
Contributor

Choose a reason for hiding this comment

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

get_interceptor_cache(...) is probably a better name

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 162 to 168
match flow {
InterceptorFlow::Egress => &self.eg_interceptors,
InterceptorFlow::Ingress => &self.in_interceptors,
}
.as_ref()
.map(|iceptors| iceptors.load())
.and_then(|iceptors| iceptors.is_empty().not().then_some(iceptors))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the .not() and the fact that you don't use ?. Why not:

Suggested change
match flow {
InterceptorFlow::Egress => &self.eg_interceptors,
InterceptorFlow::Ingress => &self.in_interceptors,
}
.as_ref()
.map(|iceptors| iceptors.load())
.and_then(|iceptors| iceptors.is_empty().not().then_some(iceptors))
let interceptors = match flow {
InterceptorFlow::Egress => &self.eg_interceptors,
InterceptorFlow::Ingress => &self.in_interceptors,
};
Some(interceptors.as_ref()?.load()).filter(|i| !i.is_empty())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants