Raise an error if max_samples is zero in advanced pub/sub #2333
Conversation
A history depth of zero makes no sense, both on publisher cache side, and on subscriber query side. There was a bug in the subscriber, as not having `sample_depth` set was preventing proper buffering. It has been fixed by considering that no `sample_depth` set means an arbitrary number of responses to the query, so responses must be buffered till the end.
|
PR missing one of the required labels: {'dependencies', 'bug', 'enhancement', 'ci', 'new feature', 'documentation', 'api-sync', 'internal', 'breaking-change'} |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2333 +/- ##
==========================================
- Coverage 72.97% 72.86% -0.12%
==========================================
Files 388 388
Lines 62200 62196 -4
==========================================
- Hits 45391 45317 -74
- Misses 16809 16879 +70 ☔ View full report in Codecov by Sentry. |
| /// | ||
| /// Panics if `depth` is zero. | ||
| #[zenoh_macros::unstable] | ||
| pub fn max_samples(mut self, depth: usize) -> Self { |
There was a problem hiding this comment.
why not accept depth as NonZeroUsize and avoid panicking inside the setter ?
There was a problem hiding this comment.
Because it would make the API not so nice: I don't think that people like writing NonZero::new(x).unwrap()/x.try_into().unwrap() everywhere. Panicking on zero would give exactly the same result as users' unwrap.
There was a problem hiding this comment.
Fixed by removing the panic
zenoh-ext/src/advanced_subscriber.rs
Outdated
| retransmission: bool, | ||
| period: Option<Period>, | ||
| history_depth: usize, | ||
| history_depth: Option<NonZeroUsize>, |
There was a problem hiding this comment.
why not keep unwrap_or'ed value of history depth here, instead unwrapping it every time in handle_sample ?
There was a problem hiding this comment.
Because I don't think it matters a lot, but I will do the change.
wyfo
changed the title
2 participants
A history depth of zero makes no sense, both on publisher cache side, and on subscriber query side.
There was a bug in the subscriber, as not having
sample_depthset was preventing proper buffering. It has been fixed by considering that nosample_depthset means an arbitrary number of responses to the query, so responses must be buffered till the end.