-
Notifications
You must be signed in to change notification settings - Fork 410
feat(common): add deferred poll timeout support #3948
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,11 +37,27 @@ use iggy_binary_protocol::requests::messages::{ | |
| FlushUnsavedBufferRequest, PollMessagesRequest, RawMessage, SendMessagesEncoder, | ||
| }; | ||
| use iggy_binary_protocol::responses::consumer_groups::SyncConsumerGroupResponse; | ||
| use std::time::Duration; | ||
|
|
||
| /// Max attempts to resolve a fenced consumer-group poll: one re-sync after the | ||
| /// coordinator rejects a stale assignment, then retry once. | ||
| const GROUP_POLL_MAX_ATTEMPTS: usize = 2; | ||
|
|
||
| struct PollGroupOptions<'a> { | ||
| consumer: &'a Consumer, | ||
| strategy: &'a PollingStrategy, | ||
| count: u32, | ||
| auto_commit: bool, | ||
| wait_timeout_us: u64, | ||
| } | ||
|
|
||
| fn duration_to_wait_timeout_us(wait_timeout: Duration) -> Result<u64, IggyError> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sub-microsecond timeouts truncate to 0 and silently become an immediate poll. round up or reject. |
||
| wait_timeout | ||
| .as_micros() | ||
| .try_into() | ||
| .map_err(|_| IggyError::InvalidNumberValue) | ||
| } | ||
|
|
||
| fn group_cache_key(stream_id: &Identifier, topic_id: &Identifier, group_id: &Identifier) -> String { | ||
| format!("{stream_id}|{topic_id}|{group_id}") | ||
| } | ||
|
|
@@ -172,11 +188,15 @@ async fn poll_group_messages<B: BinaryClient>( | |
| client: &B, | ||
| stream_id: &Identifier, | ||
| topic_id: &Identifier, | ||
| consumer: &Consumer, | ||
| strategy: &PollingStrategy, | ||
| count: u32, | ||
| auto_commit: bool, | ||
| options: PollGroupOptions<'_>, | ||
| ) -> Result<PolledMessages, IggyError> { | ||
| let PollGroupOptions { | ||
| consumer, | ||
| strategy, | ||
| count, | ||
| auto_commit, | ||
| wait_timeout_us, | ||
| } = options; | ||
| let key = group_cache_key(stream_id, topic_id, &consumer.id); | ||
| if !client.consumer_group_state().has_assignment(&key) { | ||
| sync_group_assignment(client, stream_id, topic_id, &consumer.id).await?; | ||
|
|
@@ -205,6 +225,7 @@ async fn poll_group_messages<B: BinaryClient>( | |
| strategy: polling_strategy_to_wire(strategy), | ||
| count, | ||
| auto_commit, | ||
| wait_timeout_us, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. group polls pick one assigned partition round-robin, so a wait here parks the consumer on that partition while the others may already have data. the group case needs a design answer before the field goes on the wire. |
||
| }; | ||
| match client | ||
| .send_raw_with_response(POLL_MESSAGES_CODE, request.to_bytes()) | ||
|
|
@@ -283,8 +304,33 @@ impl<B: BinaryClient> MessageClient for B { | |
| strategy: &PollingStrategy, | ||
| count: u32, | ||
| auto_commit: bool, | ||
| ) -> Result<PolledMessages, IggyError> { | ||
| self.poll_messages_with_timeout( | ||
| stream_id, | ||
| topic_id, | ||
| partition_id, | ||
| consumer, | ||
| strategy, | ||
| count, | ||
| auto_commit, | ||
| Duration::ZERO, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| async fn poll_messages_with_timeout( | ||
| &self, | ||
| stream_id: &Identifier, | ||
| topic_id: &Identifier, | ||
| partition_id: Option<u32>, | ||
| consumer: &Consumer, | ||
| strategy: &PollingStrategy, | ||
| count: u32, | ||
| auto_commit: bool, | ||
| wait_timeout: Duration, | ||
| ) -> Result<PolledMessages, IggyError> { | ||
| fail_if_not_authenticated(self).await?; | ||
| let wait_timeout_us = duration_to_wait_timeout_us(wait_timeout)?; | ||
| // VSR: a consumer-group poll without an explicit partition is resolved | ||
| // client-side from the member's cached assignment (the broker routes | ||
| // explicit partitions only). | ||
|
|
@@ -293,10 +339,13 @@ impl<B: BinaryClient> MessageClient for B { | |
| self, | ||
| stream_id, | ||
| topic_id, | ||
| consumer, | ||
| strategy, | ||
| count, | ||
| auto_commit, | ||
| PollGroupOptions { | ||
| consumer, | ||
| strategy, | ||
| count, | ||
| auto_commit, | ||
| wait_timeout_us, | ||
| }, | ||
| ) | ||
| .await; | ||
| } | ||
|
|
@@ -308,6 +357,7 @@ impl<B: BinaryClient> MessageClient for B { | |
| strategy: polling_strategy_to_wire(strategy), | ||
| count, | ||
| auto_commit, | ||
| wait_timeout_us, | ||
| }; | ||
| let response = self | ||
| .send_raw_with_response(POLL_MESSAGES_CODE, req.to_bytes()) | ||
|
|
@@ -402,9 +452,12 @@ impl<B: BinaryClient> MessageClient for B { | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{committed_send_confirmations, decode_send_confirmations}; | ||
| use super::{ | ||
| committed_send_confirmations, decode_send_confirmations, duration_to_wait_timeout_us, | ||
| }; | ||
| use crate::{IggyError, SendMessagesConfirmationResponse, SendMessagesResponse}; | ||
| use iggy_binary_protocol::codec::WireEncode; | ||
| use std::time::Duration; | ||
|
|
||
| fn response() -> SendMessagesResponse { | ||
| SendMessagesResponse { | ||
|
|
@@ -494,4 +547,24 @@ mod tests { | |
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn wait_timeout_uses_microseconds() { | ||
| assert_eq!( | ||
| duration_to_wait_timeout_us(Duration::from_millis(25)).unwrap(), | ||
| 25_000 | ||
| ); | ||
| assert_eq!( | ||
| duration_to_wait_timeout_us(Duration::from_nanos(999)).unwrap(), | ||
| 0 | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn wait_timeout_overflow_is_rejected() { | ||
| assert_eq!( | ||
| duration_to_wait_timeout_us(Duration::new(u64::MAX, 0)), | ||
| Err(IggyError::InvalidNumberValue) | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence is sniffed from the remaining length, so the next optional field has to repeat the trick. a flags byte or a version would age better.