-
Notifications
You must be signed in to change notification settings - Fork 413
feat(connectors): let sinks control when offsets are committed #3954
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ pub enum RuntimeError { | |
| FailedToSerializeMessagesMetadata, | ||
| #[error("Failed to serialize raw messages")] | ||
|
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. nit: positional tuple variant - named fields read better, and this message is the only thing a test or an operator can match on. |
||
| FailedToSerializeRawMessages, | ||
| #[error("Sink connector with ID: {0} rejected the batch with code: {1}")] | ||
| SinkRejectedBatch(u32, i32), | ||
| #[error("Connector SDK error")] | ||
| ConnectorSdkError(#[from] iggy_connector_sdk::Error), | ||
| /// A classified state-store failure while loading an enabled source's | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |
| // under the License. | ||
|
|
||
| use crate::benchmark; | ||
| use crate::configs::connectors::SinkConfig; | ||
| use crate::configs::connectors::{OffsetCommitMode, SinkConfig}; | ||
| use crate::context::RuntimeContext; | ||
| use crate::log::LOG_CALLBACK; | ||
| use crate::metrics::{Metrics, SinkLabels}; | ||
|
|
@@ -147,6 +147,7 @@ pub async fn init( | |
| error: init_error.clone(), | ||
| verbose: config.verbose, | ||
| benchmark: config.benchmark, | ||
| offset_commit: config.offset_commit, | ||
| }); | ||
|
|
||
| if let Some(error) = init_error { | ||
|
|
@@ -229,6 +230,7 @@ pub fn consume( | |
| sink.callback, | ||
| plugin.verbose, | ||
| plugin.benchmark, | ||
| plugin.offset_commit, | ||
| &context.metrics, | ||
| context.clone(), | ||
| ); | ||
|
|
@@ -251,6 +253,7 @@ pub(crate) fn spawn_consume_tasks( | |
| callback: ConsumeCallback, | ||
| verbose: bool, | ||
| benchmark: bool, | ||
| offset_commit: OffsetCommitMode, | ||
| metrics: &Arc<Metrics>, | ||
| context: Arc<RuntimeContext>, | ||
| ) -> (watch::Sender<()>, Vec<JoinHandle<()>>) { | ||
|
|
@@ -267,6 +270,7 @@ pub(crate) fn spawn_consume_tasks( | |
| let plugin_key = plugin_key.to_string(); | ||
| let metrics = metrics.clone(); | ||
| let shutdown_rx = shutdown_rx.clone(); | ||
| let shutdown_tx = shutdown_tx.clone(); | ||
| let context = context.clone(); | ||
| let labels = labels.clone(); | ||
| let handle = tokio::spawn(async move { | ||
|
|
@@ -279,6 +283,7 @@ pub(crate) fn spawn_consume_tasks( | |
| consumer, | ||
| verbose, | ||
| benchmark, | ||
| offset_commit, | ||
| &plugin_key, | ||
| &metrics, | ||
| &labels, | ||
|
|
@@ -294,6 +299,11 @@ pub(crate) fn spawn_consume_tasks( | |
| .sinks | ||
| .set_error(&plugin_key, &error.to_string()) | ||
| .await; | ||
| // The instance owns the target connection, so one topic's | ||
| // failure condemns the rest. Stopping them here keeps the | ||
| // failure domain the same as the recovery domain: the whole | ||
| // connector goes down, and `restart_connector` brings it back. | ||
| let _ = shutdown_tx.send(()); | ||
|
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. critical: a sibling task woken by this breaks out and drops its half-filled batch. under |
||
| } | ||
| }); | ||
| task_handles.push(handle); | ||
|
|
@@ -311,6 +321,7 @@ pub(crate) async fn consume_messages( | |
| mut consumer: IggyConsumer, | ||
| verbose: bool, | ||
| benchmark: bool, | ||
| offset_commit: OffsetCommitMode, | ||
| plugin_key: &str, | ||
| metrics: &Arc<Metrics>, | ||
| labels: &SinkLabels, | ||
|
|
@@ -389,6 +400,11 @@ pub(crate) async fn consume_messages( | |
| // Total always records; sub-stages only on success (no 0-sample skew). | ||
| metrics.observe_stage_with_labels(&labels.stage_total, elapsed); | ||
|
|
||
| let consume_result = match &result { | ||
| Ok(timing) => timing.consume_result, | ||
| Err(_) => 0, | ||
|
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. simplification: the |
||
| }; | ||
|
|
||
| let (processed_count, decode_us, prepare_us, ffi_us) = match &result { | ||
|
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. warning: a rejected batch takes the |
||
| Ok(timing) => { | ||
| let prepare_elapsed = elapsed | ||
|
|
@@ -430,6 +446,33 @@ pub(crate) async fn consume_messages( | |
| return Err(error); | ||
| } | ||
|
|
||
| if consume_result != 0 { | ||
| error!( | ||
| "Sink connector with ID: {plugin_id} rejected {messages_count} messages from \ | ||
| stream: {}, topic: {}, partition ID: {partition_id} with code: {consume_result}", | ||
| topic_metadata.stream, topic_metadata.topic, | ||
| ); | ||
| metrics.inc_errors_with_labels(&labels.counter); | ||
|
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. nit: counted here and again when the |
||
| // A rejection means the target is unusable, not that this batch is | ||
| // bad - a sink drops bad records itself and returns success. Both | ||
| // modes stop: continuing would hand every later batch to the same | ||
| // failing target, and under `AfterPolling` each of those is already | ||
| // committed at poll time, so the topic would drain into nothing. | ||
| return Err(RuntimeError::SinkRejectedBatch(plugin_id, consume_result)); | ||
|
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. critical: this fires under the default |
||
| } | ||
|
|
||
| if offset_commit == OffsetCommitMode::AfterConsuming | ||
|
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. warning: this commits the batch's last offset, marking everything up to it consumed - including messages already dropped by decode or transform failures. |
||
| && let Err(error) = consumer | ||
| .store_offset(message_offset, Some(partition_id)) | ||
|
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. critical: any |
||
| .await | ||
| { | ||
| error!( | ||
| "Failed to store offset: {message_offset} for partition ID: {partition_id}, \ | ||
| sink connector with ID: {plugin_id}. {error}", | ||
| ); | ||
| return Err(error.into()); | ||
| } | ||
|
|
||
| metrics.inc_messages_processed_with_labels(&labels.counter, processed_count as u64); | ||
|
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. nit: |
||
| if verbose { | ||
| info!( | ||
|
|
@@ -502,6 +545,11 @@ pub(crate) async fn setup_sink_consumers( | |
| vec![] | ||
| }; | ||
|
|
||
| let auto_commit = match config.offset_commit { | ||
| OffsetCommitMode::AfterPolling => AutoCommit::When(AutoCommitWhen::PollingMessages), | ||
| OffsetCommitMode::AfterConsuming => AutoCommit::Disabled, | ||
| }; | ||
|
|
||
| let mut consumers = Vec::new(); | ||
| for stream in config.streams.iter() { | ||
| let poll_interval = IggyDuration::from_str( | ||
|
|
@@ -519,7 +567,7 @@ pub(crate) async fn setup_sink_consumers( | |
| for topic in stream.topics.iter() { | ||
| let mut consumer = iggy_client | ||
| .consumer_group(consumer_group, &stream.stream, topic)? | ||
| .auto_commit(AutoCommit::When(AutoCommitWhen::PollingMessages)) | ||
| .auto_commit(auto_commit) | ||
| .create_consumer_group_if_not_exists() | ||
| .auto_join_consumer_group() | ||
| .polling_strategy(PollingStrategy::next()) | ||
|
|
@@ -737,7 +785,7 @@ async fn process_messages( | |
| })?; | ||
|
|
||
| let ffi_start = Instant::now(); | ||
| (consume)( | ||
| let consume_result = (consume)( | ||
|
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. nit: nothing unit-tests |
||
| plugin_id, | ||
| topic_meta.as_ptr(), | ||
| topic_meta.len(), | ||
|
|
@@ -752,11 +800,15 @@ async fn process_messages( | |
| processed_count, | ||
| decode_elapsed, | ||
| ffi_elapsed, | ||
| consume_result, | ||
| }) | ||
| } | ||
|
|
||
| struct SinkBatchTiming { | ||
| processed_count: usize, | ||
| decode_elapsed: Duration, | ||
| ffi_elapsed: Duration, | ||
| /// Plugin's `iggy_sink_consume` return code: 0 on success, non-zero when | ||
| /// the sink rejected the batch. | ||
| consume_result: i32, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| [package] | ||
| name = "iggy_connector_test_sink" | ||
| version = "0.5.0-edge.4" | ||
| description = "Sink connector plugin used only by the Iggy integration test suite to drive controllable success and failure behaviour." | ||
| edition = "2024" | ||
| license = "Apache-2.0" | ||
| publish = false | ||
|
|
||
| [package.metadata.cargo-machete] | ||
| ignored = ["dashmap"] | ||
|
|
||
| [lib] | ||
| crate-type = ["cdylib", "lib"] | ||
|
|
||
| [dependencies] | ||
| async-trait = { workspace = true } | ||
| dashmap = { workspace = true } | ||
| iggy_connector_sdk = { workspace = true } | ||
| serde = { workspace = true } | ||
| tokio = { workspace = true } | ||
| tracing = { workspace = true } | ||
|
|
||
| [lints] | ||
| workspace = true |
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.
warning:
after_consumingguarantees duplicates when the process dies between the sink accepting a batch andstore_offsetlanding. sinks have to be idempotent - worth saying in the doc comment and the README.