-
-
Notifications
You must be signed in to change notification settings - Fork 3
fix(blq): Fix forward loop in demoted namespaces #504
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
8a12967
fix(blq): Modify namespace to "long" in forwarded TaskActivation
lvthanh03 367c34f
comments
lvthanh03 d7c6fa1
Record forwarded_rows and allow flush
lvthanh03 724bae8
flush Writer if batch is empty
lvthanh03 631489b
Add tests for modifying namespaces
lvthanh03 dbe860e
Try btaching forwards
lvthanh03 5224077
Only batch forwards
lvthanh03 24c9f4d
upkeep metrics
lvthanh03 ccbe793
import order
lvthanh03 45b83d8
Add flush results tests
lvthanh03 d9fb271
Add more metrics for forwarding tasks
lvthanh03 4af1359
compare topics instead of tagging
lvthanh03 2aed06d
forward logic fix
lvthanh03 31833ef
Produce zeros for consumer.batch_rows and batch_bytes metrics
lvthanh03 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ use crate::{ | |
| store::inflight_activation::InflightActivation, | ||
| }; | ||
| use chrono::Utc; | ||
| use futures::future::join_all; | ||
| use rdkafka::config::ClientConfig; | ||
| use rdkafka::{ | ||
| producer::{FutureProducer, FutureRecord}, | ||
|
|
@@ -17,6 +18,7 @@ use super::consumer::{ | |
|
|
||
| pub struct ActivationBatcherConfig { | ||
| pub kafka_config: ClientConfig, | ||
| pub kafka_topic: String, | ||
| pub kafka_long_topic: String, | ||
| pub send_timeout_ms: u64, | ||
| pub max_batch_time_ms: u64, | ||
|
|
@@ -29,6 +31,7 @@ impl ActivationBatcherConfig { | |
| pub fn from_config(config: &Config) -> Self { | ||
| Self { | ||
| kafka_config: config.kafka_producer_config(), | ||
| kafka_topic: config.kafka_topic.clone(), | ||
| kafka_long_topic: config.kafka_long_topic.clone(), | ||
| send_timeout_ms: config.kafka_send_timeout_ms, | ||
| max_batch_time_ms: config.db_insert_batch_max_time_ms, | ||
|
|
@@ -41,6 +44,7 @@ impl ActivationBatcherConfig { | |
| pub struct InflightActivationBatcher { | ||
| batch: Vec<InflightActivation>, | ||
| batch_size: usize, | ||
| forward_batch: Vec<Vec<u8>>, // payload | ||
| config: ActivationBatcherConfig, | ||
| runtime_config_manager: Arc<RuntimeConfigManager>, | ||
| producer: Arc<FutureProducer>, | ||
|
|
@@ -60,6 +64,7 @@ impl InflightActivationBatcher { | |
| Self { | ||
| batch: Vec::with_capacity(config.max_batch_len), | ||
| batch_size: 0, | ||
| forward_batch: Vec::with_capacity(config.max_batch_len), | ||
| config, | ||
| runtime_config_manager, | ||
| producer, | ||
|
|
@@ -74,6 +79,10 @@ impl Reducer for InflightActivationBatcher { | |
|
|
||
| async fn reduce(&mut self, t: Self::Input) -> Result<(), anyhow::Error> { | ||
| let runtime_config = self.runtime_config_manager.read().await; | ||
| let forward_topic = runtime_config | ||
| .demoted_topic | ||
| .clone() | ||
| .unwrap_or(self.config.kafka_long_topic.clone()); | ||
| let task_name = &t.taskname; | ||
| let namespace = &t.namespace; | ||
|
|
||
|
|
@@ -91,31 +100,21 @@ impl Reducer for InflightActivationBatcher { | |
| } | ||
|
|
||
| if runtime_config.demoted_namespaces.contains(namespace) { | ||
| metrics::counter!( | ||
| "filter.forward_task_demoted_namespace", | ||
| "namespace" => namespace.clone(), | ||
| "taskname" => task_name.clone(), | ||
| ) | ||
| .increment(1); | ||
|
|
||
| // The default demoted topic to forward tasks to is config.kafka_long_topic if not set in runtime config. | ||
| let topic = runtime_config | ||
| .demoted_topic | ||
| .clone() | ||
| .unwrap_or(self.config.kafka_long_topic.clone()); | ||
| let delivery = self | ||
| .producer | ||
| .send( | ||
| FutureRecord::<(), Vec<u8>>::to(&topic).payload(&t.activation), | ||
| Timeout::After(Duration::from_millis(self.config.send_timeout_ms)), | ||
| if forward_topic == self.config.kafka_topic { | ||
| metrics::counter!( | ||
| "filter.forward_task_demoted_namespace.skipped", | ||
| "namespace" => namespace.clone(), | ||
| "taskname" => task_name.clone(), | ||
| ) | ||
| .await; | ||
| if delivery.is_ok() { | ||
| metrics::counter!("filter.forward_task_demoted_namespace_success", | ||
| .increment(1); | ||
| } else { | ||
| metrics::counter!( | ||
| "filter.forward_task_demoted_namespace", | ||
| "namespace" => namespace.clone(), | ||
| "taskname" => task_name.clone(), | ||
| ) | ||
| .increment(1); | ||
| self.forward_batch.push(t.activation.clone()); | ||
| return Ok(()); | ||
| } | ||
| } | ||
|
|
@@ -127,13 +126,38 @@ impl Reducer for InflightActivationBatcher { | |
| } | ||
|
|
||
| async fn flush(&mut self) -> Result<Option<Self::Output>, anyhow::Error> { | ||
| if self.batch.is_empty() { | ||
| if self.batch.is_empty() && self.forward_batch.is_empty() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| metrics::histogram!("consumer.batch_rows").record(self.batch.len() as f64); | ||
| metrics::histogram!("consumer.batch_bytes").record(self.batch_size as f64); | ||
|
|
||
| // Send all forward batch in parallel | ||
| if !self.forward_batch.is_empty() { | ||
| let runtime_config = self.runtime_config_manager.read().await; | ||
| let forward_topic = runtime_config | ||
| .demoted_topic | ||
| .clone() | ||
| .unwrap_or(self.config.kafka_long_topic.clone()); | ||
| let sends = self.forward_batch.iter().map(|payload| { | ||
| self.producer.send( | ||
| FutureRecord::<(), Vec<u8>>::to(&forward_topic).payload(payload), | ||
| Timeout::After(Duration::from_millis(self.config.send_timeout_ms)), | ||
| ) | ||
| }); | ||
|
|
||
| let results = join_all(sends).await; | ||
| let success_count = results.iter().filter(|r| r.is_ok()).count(); | ||
|
|
||
| metrics::histogram!("consumer.forward_attempts").record(results.len() as f64); | ||
| metrics::histogram!("consumer.forward_successes").record(success_count as f64); | ||
| metrics::histogram!("consumer.forward_failures") | ||
| .record((results.len() - success_count) as f64); | ||
|
|
||
| self.forward_batch.clear(); | ||
| } | ||
|
|
||
| self.batch_size = 0; | ||
|
|
||
| Ok(Some(replace( | ||
|
|
@@ -144,6 +168,7 @@ impl Reducer for InflightActivationBatcher { | |
|
|
||
| fn reset(&mut self) { | ||
| self.batch_size = 0; | ||
| self.forward_batch.clear(); | ||
| self.batch.clear(); | ||
| } | ||
|
|
||
|
|
@@ -428,6 +453,8 @@ demoted_namespaces: | |
| ActivationBatcherConfig::from_config(&config), | ||
| runtime_config, | ||
| ); | ||
| println!("kafka_topic: {:?}", config.kafka_topic); | ||
| println!("kafka_long_topic: {:?}", config.kafka_long_topic); | ||
|
Comment on lines
+456
to
+457
Member
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. Some stray prints. |
||
|
|
||
| let inflight_activation_0 = InflightActivation { | ||
| id: "0".to_string(), | ||
|
|
@@ -460,8 +487,53 @@ demoted_namespaces: | |
| on_attempts_exceeded: OnAttemptsExceeded::Discard, | ||
| }; | ||
|
|
||
| let inflight_activation_1 = InflightActivation { | ||
| id: "1".to_string(), | ||
| activation: TaskActivation { | ||
| id: "1".to_string(), | ||
| namespace: "good_namespace".to_string(), | ||
| taskname: "good_task".to_string(), | ||
| parameters: "{}".to_string(), | ||
| headers: HashMap::new(), | ||
| received_at: None, | ||
| retry_state: None, | ||
| processing_deadline_duration: 0, | ||
| expires: Some(0), | ||
| delay: None, | ||
| } | ||
| .encode_to_vec(), | ||
| status: InflightActivationStatus::Pending, | ||
| partition: 0, | ||
| offset: 0, | ||
| added_at: Utc::now(), | ||
| received_at: Utc::now(), | ||
| processing_attempts: 0, | ||
| processing_deadline_duration: 0, | ||
| expires_at: None, | ||
| delay_until: None, | ||
| processing_deadline: None, | ||
| at_most_once: false, | ||
| namespace: "good_namespace".to_string(), | ||
| taskname: "good_task".to_string(), | ||
| on_attempts_exceeded: OnAttemptsExceeded::Discard, | ||
| }; | ||
|
|
||
| batcher.reduce(inflight_activation_0).await.unwrap(); | ||
| batcher.reduce(inflight_activation_1).await.unwrap(); | ||
|
|
||
| assert_eq!(batcher.batch.len(), 1); | ||
| assert_eq!(batcher.forward_batch.len(), 1); | ||
|
|
||
| let flush_result = batcher.flush().await.unwrap(); | ||
| assert!(flush_result.is_some()); | ||
| assert_eq!(flush_result.as_ref().unwrap().len(), 1); | ||
| assert_eq!( | ||
| flush_result.as_ref().unwrap()[0].namespace, | ||
| "good_namespace" | ||
| ); | ||
| assert_eq!(flush_result.as_ref().unwrap()[0].taskname, "good_task"); | ||
| assert_eq!(batcher.batch.len(), 0); | ||
| assert_eq!(batcher.forward_batch.len(), 0); | ||
|
|
||
| fs::remove_file(test_path).await.unwrap(); | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Much better 👏