From b481724eec764091ada89241458e2cf0b57c60a9 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Tue, 25 Aug 2026 05:12:03 +0530 Subject: [PATCH 01/10] Templated code initial version --- Cargo.lock | 36 ++ Cargo.toml | 2 + core/connectors/BLOG_POST.md | 104 ++++ .../connectors/sink_template.toml | 47 ++ .../connectors/source_template.toml | 46 ++ core/connectors/sinks/README.md | 1 + .../connectors/sinks/sink_template/Cargo.toml | 58 ++ core/connectors/sinks/sink_template/README.md | 70 +++ .../sinks/sink_template/config.toml | 48 ++ .../connectors/sinks/sink_template/src/lib.rs | 504 +++++++++++++++ core/connectors/sources/README.md | 1 + .../sources/source_template/Cargo.toml | 60 ++ .../sources/source_template/README.md | 64 ++ .../sources/source_template/config.toml | 47 ++ .../sources/source_template/src/lib.rs | 586 ++++++++++++++++++ 15 files changed, 1674 insertions(+) create mode 100644 core/connectors/BLOG_POST.md create mode 100644 core/connectors/runtime/example_config/connectors/sink_template.toml create mode 100644 core/connectors/runtime/example_config/connectors/source_template.toml create mode 100644 core/connectors/sinks/sink_template/Cargo.toml create mode 100644 core/connectors/sinks/sink_template/README.md create mode 100644 core/connectors/sinks/sink_template/config.toml create mode 100644 core/connectors/sinks/sink_template/src/lib.rs create mode 100644 core/connectors/sources/source_template/Cargo.toml create mode 100644 core/connectors/sources/source_template/README.md create mode 100644 core/connectors/sources/source_template/config.toml create mode 100644 core/connectors/sources/source_template/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index bc9698629a..fa3a3efb13 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7226,6 +7226,42 @@ dependencies = [ "tracing", ] +[[package]] +name = "iggy_connector_template_sink" +version = "0.1.0" +dependencies = [ + "async-trait", + "dashmap", + "iggy_common", + "iggy_connector_sdk", + "reqwest 0.13.4", + "reqwest-middleware", + "secrecy", + "serde", + "serde_json", + "tokio", + "tracing", +] + +[[package]] +name = "iggy_connector_template_source" +version = "0.1.0" +dependencies = [ + "async-trait", + "dashmap", + "humantime", + "iggy_common", + "iggy_connector_sdk", + "reqwest 0.13.4", + "reqwest-middleware", + "rmp-serde", + "secrecy", + "serde", + "serde_json", + "tokio", + "tracing", +] + [[package]] name = "iggy_examples" version = "0.0.6" diff --git a/Cargo.toml b/Cargo.toml index 073e46a65d..7ef6257a2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,12 +45,14 @@ members = [ "core/connectors/sinks/postgres_sink", "core/connectors/sinks/quickwit_sink", "core/connectors/sinks/s3_sink", + "core/connectors/sinks/sink_template", "core/connectors/sinks/stdout_sink", "core/connectors/sinks/surrealdb_sink", "core/connectors/sources/elasticsearch_source", "core/connectors/sources/influxdb_source", "core/connectors/sources/postgres_source", "core/connectors/sources/random_source", + "core/connectors/sources/source_template", "core/consensus", "core/cpu_allocation", "core/harness_derive", diff --git a/core/connectors/BLOG_POST.md b/core/connectors/BLOG_POST.md new file mode 100644 index 0000000000..fe8b45ea1c --- /dev/null +++ b/core/connectors/BLOG_POST.md @@ -0,0 +1,104 @@ +# Announcing sink and source connector templates + +*Draft for the Apache Iggy project blog. Replace this header line with +the final publish date and author byline before posting.* + +Apache Iggy's connectors subsystem has grown fast. In the past few +months alone, contributors have shipped or proposed sink and source +connectors for Postgres, MongoDB, Elasticsearch, Iceberg, Delta Lake, +S3, InfluxDB, Doris, ClickHouse, SurrealDB, Meilisearch, OpenSearch, +Redshift, and more — each one a plugin that moves real data between +Apache Iggy and an external system, often in production. That growth +is great news for the project. It also means new contributors keep +re-solving the same non-backend-specific problems from scratch before +their PR can even get to the interesting part: talking to their actual +system. + +## What we found + +Looking back across recent connector PR reviews, the same handful of +issues came up again and again, and none of them had anything to do +with the destination or source system being integrated: + +- **Credentials typed as plain `String`.** Connection strings and API + keys landing in `Debug`/log output because the field wasn't + `secrecy::SecretString`. +- **Cursor commits that don't survive a failed delivery.** Since + [#3855](https://github.com/apache/iggy/pull/3855), sources use a + formal ACK/NACK handshake — `poll()` stages candidate state, and + only `on_batch_result()` commits it — but that shape has to be + learned and wired up correctly every time. +- **Errors that don't distinguish retry-worthy from permanent.** + Network hiccups and "this payload will never be accepted" ending up + in the same catch-all error variant. +- **Config knobs with drifting names.** `retry_max_delay` here, + `max_retry_delay` there, `request_timeout` somewhere else, for the + same concept. +- **Missing canonical tests.** State restore/round-trip and ACK/NACK + commit/discard behavior left untested because nobody had a reference + test suite to copy. + +None of this is specific to any one backend. It's framework plumbing +that every sink and every source needs, and until now every author +either copied the closest existing plugin and stripped it down, or +started from a blank `lib.rs` and rediscovered each of these the hard +way, one review round at a time. + +## The templates + +[`core/connectors/sinks/sink_template`](sinks/sink_template) and +[`core/connectors/sources/source_template`](sources/source_template) +are compiling, tested crates you copy and fill in — not prose +describing a pattern, but the pattern itself, already wired up and +passing `cargo test`: + +- Config parsing with `#[serde(deny_unknown_fields)]`, so a typo'd TOML + key fails loudly instead of silently doing nothing. +- `SecretString` on every credential-shaped field + (`connection_string`, `auth_token`), via + `iggy_common::serde_secret::serialize_secret`. +- A retry-wrapped HTTP client plus a startup connectivity probe with + its own backoff. +- A `CircuitBreaker` that's actually consulted before each call and + updated once per `consume()`/`poll()`, not per chunk. +- Sink: batching by a configurable size, and a `last_err` pattern that + never swallows a failed batch into `Ok(())`. +- Source: the full #3855 ACK/NACK contract — `poll()` stages a + candidate cursor, `on_batch_result()` commits it on `Ack` or + discards it on `Nack`, so a dropped batch gets re-polled instead of + silently lost. +- The canonical test suites: six tests for the sink, eight for the + source (four state tests — restore, no-state, invalid-state, + round-trip — plus the two ACK/NACK tests, plus config validation and + the circuit-breaker short-circuit path). + +What's left is marked `TODO(Developer)` in each crate's `src/lib.rs`: +one spot for a sink (`push_batch()`), two for a source +(`build_raw_client()` if you're not talking HTTP, and +`fetch_records()`). Everything else — the parts that used to eat a +review round — is already done. + +## Using one + +Copy the crate, rename the package and the directory, add it to the +workspace `members` list, fill in the `TODO(Developer)` spots, and +update `config.toml` for your system. Each crate's own `README.md` +walks through the exact steps. Both templates already build, `clippy +--all-targets -- -D warnings` clean, and pass their tests as committed +— the only thing that should break when you fill in the TODOs is the +`Err(Error::InitError("not implemented yet"))` stub they start from. + +## Why this matters beyond Apache Iggy's connectors + +The pattern generalizes past this one subsystem: any plugin system +with a real framework contract — secrets, retries, staged +commit/rollback, canonical tests — benefits more from a working, +compiling example than from a checklist alone. A checklist tells you +what to verify; a template gives you the thing already verified, so +your own diff is just the part only you can write. + +--- + +*Feedback and discussion: see the project's +[GitHub Discussions](https://github.com/apache/iggy/discussions) or +[Discord](https://discord.gg/apache-iggy).* diff --git a/core/connectors/runtime/example_config/connectors/sink_template.toml b/core/connectors/runtime/example_config/connectors/sink_template.toml new file mode 100644 index 0000000000..90c3937f95 --- /dev/null +++ b/core/connectors/runtime/example_config/connectors/sink_template.toml @@ -0,0 +1,47 @@ +# 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. + +type = "sink" +key = "template" +enabled = true +version = 0 +name = "Template sink" +path = "target/release/libiggy_connector_template_sink" +plugin_config_format = "toml" +verbose = false + +[[streams]] +stream = "example_stream" +topics = ["example_topic"] +schema = "json" +batch_length = 100 +poll_interval = "5ms" +consumer_group = "template_sink_connector" + +[plugin_config] +connection_string = "https://api.example.com" +target = "events" +health_check_path = "/health" +batch_size = 100 +timeout = "30s" +max_retries = 3 +retry_delay = "500ms" +retry_max_delay = "5s" +max_open_retries = 10 +open_retry_max_delay = "60s" +circuit_breaker_threshold = 5 +circuit_breaker_cool_down = "30s" diff --git a/core/connectors/runtime/example_config/connectors/source_template.toml b/core/connectors/runtime/example_config/connectors/source_template.toml new file mode 100644 index 0000000000..c53439a0ae --- /dev/null +++ b/core/connectors/runtime/example_config/connectors/source_template.toml @@ -0,0 +1,46 @@ +# 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. + +type = "source" +key = "template" +enabled = true +version = 0 +name = "Template source" +path = "target/release/libiggy_connector_template_source" +plugin_config_format = "toml" +verbose = false + +[[streams]] +stream = "example_stream" +topic = "example_topic" +schema = "json" +batch_length = 100 +linger_time = "5ms" + +[plugin_config] +connection_string = "https://api.example.com" +health_check_path = "/health" +batch_size = 100 +poll_interval = "1s" +timeout = "30s" +max_retries = 3 +retry_delay = "500ms" +retry_max_delay = "5s" +max_open_retries = 10 +open_retry_max_delay = "60s" +circuit_breaker_threshold = 5 +circuit_breaker_cool_down = "30s" diff --git a/core/connectors/sinks/README.md b/core/connectors/sinks/README.md index e23e1ace9c..990cbe59f8 100644 --- a/core/connectors/sinks/README.md +++ b/core/connectors/sinks/README.md @@ -16,6 +16,7 @@ Sink connectors are responsible for writing data from Iggy streams to external s | **postgres_sink** | Stores messages in PostgreSQL database tables with configurable schemas | | **quickwit_sink** | Indexes messages in Quickwit search engine for log analytics | | **s3_sink** | Writes messages to Amazon S3 and S3-compatible stores (MinIO, R2, B2, DO Spaces) | +| **sink_template** | Fill-in-the-blank starting point for a new sink; framework/security plumbing done, one `TODO(Developer)` spot left | | **stdout_sink** | Prints messages to standard output (useful for debugging and development) | | **surrealdb_sink** | Writes messages into SurrealDB with deterministic record IDs for idempotent replay | diff --git a/core/connectors/sinks/sink_template/Cargo.toml b/core/connectors/sinks/sink_template/Cargo.toml new file mode 100644 index 0000000000..8ab161bd1e --- /dev/null +++ b/core/connectors/sinks/sink_template/Cargo.toml @@ -0,0 +1,58 @@ +# 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. +# +# TEMPLATE — rename the package (and this directory) to +# `iggy_connector__sink` before publishing, and update the +# `[[sinks]]` entry you add to the workspace root Cargo.toml accordingly. + +[package] +name = "iggy_connector_template_sink" +version = "0.1.0" +description = "Template for an Apache Iggy sink connector — copy this crate and fill in the TODO sections." +edition = "2024" +license = "Apache-2.0" +keywords = ["iggy", "messaging", "streaming"] +categories = ["command-line-utilities", "database", "network-programming"] +homepage = "https://iggy.apache.org" +documentation = "https://iggy.apache.org/docs" +repository = "https://github.com/apache/iggy" +publish = false + +[package.metadata.cargo-machete] +# dashmap is used only inside the `sink_connector!` macro expansion, so a +# naive unused-dependency scan won't see the usage — keep it ignored rather +# than removing it, or the plugin will fail to compile. +ignored = ["dashmap"] + +[lib] +crate-type = ["cdylib", "lib"] + +[dependencies] +async-trait = { workspace = true } +dashmap = { workspace = true } +iggy_common = { workspace = true } +iggy_connector_sdk = { workspace = true } +reqwest = { workspace = true } +reqwest-middleware = { workspace = true } +secrecy = { workspace = true } +serde = { workspace = true } +serde_json = { workspace = true } +tokio = { workspace = true } +tracing = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true, features = ["test-util"] } diff --git a/core/connectors/sinks/sink_template/README.md b/core/connectors/sinks/sink_template/README.md new file mode 100644 index 0000000000..16547f5d14 --- /dev/null +++ b/core/connectors/sinks/sink_template/README.md @@ -0,0 +1,70 @@ +# Template sink connector + +Starting point for a new Apache Iggy **sink** connector. Everything except +pushing data to your actual destination is already implemented and follows +the project's required resilience/security patterns — see the module-level +doc comment at the top of `src/lib.rs` for the full rationale, and the +`iggy-connector-review` skill / the "Building Connectors That Pass Review" +blog post for the checklist this template is built against. + +## What's already done for you + +- Config parsing with `#[serde(deny_unknown_fields)]` so a typo in a TOML + file fails loudly instead of silently doing nothing. +- Config validation in `open()` (not `new()`, which has no way to return an + error) — including validating `target` (the destination table/index/ + collection name) against an allowlist pattern *before* it can ever reach a + query, path, or URL. +- `connection_string` and the optional `auth_token` field both typed as + `SecretString`, since either can carry credentials. +- A retry-wrapped HTTP client (`iggy_connector_sdk::retry::build_retry_client`) + and a startup connectivity probe with its own backoff + (`check_connectivity_with_retry`). +- A `CircuitBreaker` that's actually consulted before each `consume()` call + and updated once per call based on the outcome — not just constructed and + forgotten, and not reset mid-batch by a partial success. +- Batching: `consume()` chunks the incoming messages by a configurable + `batch_size` instead of sending everything in one unbounded request. +- The `sink_connector!` FFI macro invocation and a `Cargo.toml` with the + right `crate-type`, workspace-pinned dependencies, and license header. +- Tests for config/identifier validation and the circuit-breaker short-circuit + path. + +## What you need to fill in + +Search for `TODO(Developer)` in `src/lib.rs` — there is exactly one spot: + +**`push_batch()`** — build the request/write that actually sends one chunk +of messages to your destination, using `self.config.connection_string` (and +`self.config.target`, already validated by the time this runs) via +`self.client` (already retry-wrapped). Distinguish permanent failures (bad +schema, a destination that will reject this payload shape no matter how many +times you retry) from transient ones (network error, 5xx, timeout) by +returning `Error::PermanentHttpError` for the former — see the doc comment on +that variant for why the distinction matters to the circuit breaker. + +If your destination isn't HTTP, also revisit **`build_raw_client()`**: swap +the `reqwest::Client` for your driver's connection/pool setup (see +`core/connectors/sinks/postgres_sink` or `core/connectors/sinks/s3_sink` for +non-HTTP examples), store it on `TemplateSink` in place of the HTTP-specific +`client` field, and adjust or remove the `check_connectivity_with_retry` call +in `open()` in favor of whatever connectivity check your driver offers. + +## Using it + +1. Copy this directory, rename it and the package in `Cargo.toml` + (`iggy_connector__sink`), and add it to the `members` list in + the workspace root `Cargo.toml`. +2. Fill in the `TODO(Developer)` section(s). +3. Update `config.toml` with your real `connection_string` and `target`, and + any settings specific to your system; delete `auth_token` if you don't + need it, or add fields of your own the same way (see + `TemplateSinkConfig`). +4. `cargo build --release -p iggy_connector__sink`, point a + runtime connector config file's `path` at the built `.so`/`.dylib`/`.dll`, + and run the connector runtime — see `core/connectors/README.md` in this + repo for the full runtime quick-start. +5. Before opening a PR: `cargo test`, `cargo clippy --all-targets`, + `cargo fmt --check`, and re-read the connector-review checklist once more + with fresh eyes — most review round-trips come from one of the items in + that list, not from the connector-specific logic in `push_batch()`. diff --git a/core/connectors/sinks/sink_template/config.toml b/core/connectors/sinks/sink_template/config.toml new file mode 100644 index 0000000000..8156530c9d --- /dev/null +++ b/core/connectors/sinks/sink_template/config.toml @@ -0,0 +1,48 @@ +# 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. + +type = "sink" +key = "template" +enabled = true +version = 0 +name = "Template sink" +path = "../../target/release/libiggy_connector_template_sink" +plugin_config_format = "toml" +verbose = false + +[[streams]] +stream = "example_stream" +topics = ["example_topic"] +schema = "json" +batch_length = 100 +poll_interval = "5ms" +consumer_group = "template_sink_connector" + +[plugin_config] +connection_string = "https://api.example.com" +target = "events" +health_check_path = "/health" +batch_size = 100 +timeout = "30s" +max_retries = 3 +retry_delay = "500ms" +retry_max_delay = "5s" +max_open_retries = 10 +open_retry_max_delay = "60s" +circuit_breaker_threshold = 5 +circuit_breaker_cool_down = "30s" +# auth_token = "replace-me" # uncomment if your destination needs bearer/API-key auth diff --git a/core/connectors/sinks/sink_template/src/lib.rs b/core/connectors/sinks/sink_template/src/lib.rs new file mode 100644 index 0000000000..20a28aa9d6 --- /dev/null +++ b/core/connectors/sinks/sink_template/src/lib.rs @@ -0,0 +1,504 @@ +// 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. + +//! Template Apache Iggy **sink** connector. +//! +//! Everything in this file is already wired up and follows the patterns the +//! connector-review checklist expects: config validation happens in +//! `open()`, secrets are `SecretString`, the destination identifier is +//! validated before it's ever interpolated into a request, outbound calls +//! go through a retry-wrapped client plus a circuit breaker, and messages +//! are chunked by a configurable batch size instead of shipped as one +//! unbounded request. +//! +//! There is exactly **one** place you need to touch, marked `TODO(Developer)`: +//! `TemplateSink::push_batch()` — build the request/write that actually +//! pushes one chunk of messages to your destination, using +//! `self.config.connection_string` (and `self.config.target`, if your +//! destination has a table/index/collection-shaped name). +//! +//! This template assumes an HTTP-ish destination and uses `reqwest` wrapped +//! by the SDK's retry middleware, because that's what +//! `iggy_connector_sdk::retry` is built for and it covers the common case. +//! If your destination talks something else (a database, a queue, object +//! storage), swap the client type in `connect()`/`push_batch()` for your +//! driver of choice and lean on its own retry/pooling behavior — keep the +//! surrounding shape (validation in `open()`, circuit breaker, batching, +//! identifier validation) unchanged. See `core/connectors/sinks/postgres_sink` +//! or `core/connectors/sinks/s3_sink` in this repo for non-HTTP examples of +//! that same shape. + +use async_trait::async_trait; +use iggy_connector_sdk::retry::{ + CircuitBreaker, ConnectivityConfig, build_retry_client, check_connectivity_with_retry, + parse_duration, +}; +use iggy_connector_sdk::{ + ConsumedMessage, Error, MessagesMetadata, Sink, TopicMetadata, sink_connector, +}; +use reqwest::Url; +use reqwest_middleware::ClientWithMiddleware; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize}; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Duration; +use tokio::sync::Mutex; +use tracing::{error, info, warn}; + +sink_connector!(TemplateSink); + +const CONNECTOR_NAME: &str = "Template sink"; + +const DEFAULT_BATCH_SIZE: usize = 100; +const DEFAULT_TIMEOUT: &str = "30s"; +const DEFAULT_MAX_RETRIES: u32 = 3; +const DEFAULT_RETRY_DELAY: &str = "500ms"; +const DEFAULT_RETRY_MAX_DELAY: &str = "5s"; +const DEFAULT_MAX_OPEN_RETRIES: u32 = 10; +const DEFAULT_OPEN_RETRY_MAX_DELAY: &str = "60s"; +const DEFAULT_CIRCUIT_BREAKER_THRESHOLD: u32 = 5; +const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; + +// ── Configuration ─────────────────────────────────────────────────────────── +// +// Every tunable except `connection_string` and `target` is optional with a +// sane default, and unknown keys are rejected outright so a typo in a TOML +// file fails at load time instead of silently doing nothing. + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct TemplateSinkConfig { + /// TODO(Developer): document the exact shape this connector expects, e.g. + /// "https://api.example.com" or "postgres://user:pass@host:5432/db". + /// `SecretString` because DSNs commonly embed credentials — never plain + /// `String` for this field, see `PostgresSinkConfig::connection_string` + /// in `sinks/postgres_sink` for the same pattern. + #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] + pub connection_string: SecretString, + + /// The destination table/index/collection/bucket name. Kept as its own + /// field (rather than folded into `connection_string`) specifically so + /// it can be validated in `open()` before ever being interpolated into + /// a query, path, or URL — see `validate_identifier` below. Delete this + /// field if your destination has no such dynamic identifier. + pub target: String, + + /// Example of a secret-shaped setting. `SecretString` keeps it out of + /// `Debug`/log output; delete this field if `connection_string` already + /// carries all required auth. Read it with `.expose_secret()` (from the + /// `secrecy::ExposeSecret` trait) at the one place you actually need the + /// plaintext — e.g. when building an auth header in `connect()`. + #[serde( + default, + serialize_with = "iggy_common::serde_secret::serialize_optional_secret" + )] + pub auth_token: Option, + + /// Optional path (e.g. "/health") probed with retry during `open()` + /// before the connector is considered ready. Leave unset if your target + /// has no health endpoint — the probe is skipped, not failed, in that case. + pub health_check_path: Option, + + pub batch_size: Option, + pub timeout: Option, + pub max_retries: Option, + pub retry_delay: Option, + pub retry_max_delay: Option, + pub max_open_retries: Option, + pub open_retry_max_delay: Option, + pub circuit_breaker_threshold: Option, + pub circuit_breaker_cool_down: Option, +} + +/// Rejects anything that isn't a plain alphanumeric/underscore identifier. +/// Adjust the allowed character set to whatever your destination's naming +/// rules actually are, but always validate *something* before a +/// config-or-message-derived name is interpolated into a query, path, or +/// URL — see `doris_sink::validate_identifier` / `surrealdb_sink::validate_identifier` +/// in this repo for the same pattern applied to a real destination. +fn validate_identifier(field: &str, value: &str) -> Result<(), Error> { + if value.is_empty() || !value.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { + return Err(Error::InvalidConfigValue(format!( + "{field} must be non-empty and contain only ASCII alphanumeric characters and \ + underscores, got: {value:?}" + ))); + } + Ok(()) +} + +// ── Internal state ────────────────────────────────────────────────────────── + +#[derive(Debug, Default)] +struct State { + invocations_count: u64, + messages_written: u64, + messages_failed: u64, +} + +#[derive(Debug)] +pub struct TemplateSink { + id: u32, + config: TemplateSinkConfig, + client: Option, + circuit_breaker: Arc, + batch_size_limit: usize, + retry_delay: Duration, + state: Mutex, + records_written_total: AtomicU64, +} + +impl TemplateSink { + pub fn new(id: u32, config: TemplateSinkConfig) -> Self { + let retry_delay = parse_duration(config.retry_delay.as_deref(), DEFAULT_RETRY_DELAY); + let circuit_breaker = Arc::new(CircuitBreaker::new( + config + .circuit_breaker_threshold + .unwrap_or(DEFAULT_CIRCUIT_BREAKER_THRESHOLD), + parse_duration( + config.circuit_breaker_cool_down.as_deref(), + DEFAULT_CIRCUIT_BREAKER_COOL_DOWN, + ), + )); + let batch_size_limit = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE).max(1); + + Self { + id, + config, + client: None, + circuit_breaker, + batch_size_limit, + retry_delay, + state: Mutex::new(State::default()), + records_written_total: AtomicU64::new(0), + } + } + + /// TODO(Developer): build your actual client/connection here using + /// `self.config.connection_string` (and `self.config.auth_token`, if + /// your destination needs it). This template builds a plain + /// `reqwest::Client` to hand to `build_retry_client` — if you're not + /// talking HTTP, replace this with e.g. a database connection pool or + /// your driver's equivalent, store it on `self` (add a field, since + /// `client` here is HTTP-specific), and skip the + /// `check_connectivity_with_retry` call below in favor of whatever your + /// driver offers (a ping, a test query). + fn build_raw_client(&self) -> Result { + let timeout = parse_duration(self.config.timeout.as_deref(), DEFAULT_TIMEOUT); + reqwest::Client::builder() + .timeout(timeout) + .build() + .map_err(|e| Error::Connection(format!("failed to build HTTP client: {e}"))) + } + + /// TODO(Developer): push one chunk of already-batched messages to your + /// destination using `self.config.connection_string` and + /// `self.config.target`, via `self.client` (already retry-wrapped). + /// Distinguish permanent failures (bad schema, destination rejects the + /// payload shape — will not succeed on retry) from transient ones + /// (network error, 5xx, timeout — should retry) by returning + /// `Error::PermanentHttpError` for the former; see the doc comment on + /// that variant in `iggy_connector_sdk::Error` for why the distinction + /// matters to the circuit breaker. + async fn push_batch( + &self, + client: &ClientWithMiddleware, + batch: &[ConsumedMessage], + ) -> Result<(), Error> { + let _ = (client, batch); // remove once implemented + Err(Error::InitError( + "TemplateSink::push_batch is not implemented yet — see the TODO(Developer) comment in \ + template_sink/src/lib.rs" + .to_string(), + )) + } +} + +// ── Sink trait ──────────────────────────────────────────────────────────────── + +#[async_trait] +impl Sink for TemplateSink { + async fn open(&mut self) -> Result<(), Error> { + // Structural validation happens here, not in `new()`, because only + // `open()` can return an error — `new()` is a plain factory function + // with nowhere to send a "this config is invalid" result. + if self + .config + .connection_string + .expose_secret() + .trim() + .is_empty() + { + return Err(Error::InvalidConfigValue( + "connection_string must not be empty".to_string(), + )); + } + validate_identifier("target", &self.config.target)?; + + info!( + "Opening {CONNECTOR_NAME} connector with ID: {}, target: {}, batch_size: {}", + self.id, self.config.target, self.batch_size_limit + ); + + let raw_client = self.build_raw_client()?; + + if let Some(health_path) = &self.config.health_check_path { + let base = Url::parse(self.config.connection_string.expose_secret()).map_err(|e| { + Error::InvalidConfigValue(format!("connection_string is not a valid URL: {e}")) + })?; + let health_url = base.join(health_path).map_err(|e| { + Error::InvalidConfigValue(format!("invalid health_check_path: {e}")) + })?; + check_connectivity_with_retry( + &raw_client, + health_url, + CONNECTOR_NAME, + self.id, + &ConnectivityConfig { + max_open_retries: self + .config + .max_open_retries + .unwrap_or(DEFAULT_MAX_OPEN_RETRIES), + open_retry_max_delay: parse_duration( + self.config.open_retry_max_delay.as_deref(), + DEFAULT_OPEN_RETRY_MAX_DELAY, + ), + retry_delay: self.retry_delay, + }, + ) + .await?; + } else { + warn!( + "{CONNECTOR_NAME} connector with ID: {} has no health_check_path configured — \ + skipping the startup connectivity probe. Consider adding one.", + self.id + ); + } + + self.client = Some(build_retry_client( + raw_client, + self.config + .max_retries + .unwrap_or(DEFAULT_MAX_RETRIES) + .max(1), + self.retry_delay, + parse_duration( + self.config.retry_max_delay.as_deref(), + DEFAULT_RETRY_MAX_DELAY, + ), + CONNECTOR_NAME, + )); + + info!( + "{CONNECTOR_NAME} connector with ID: {} opened successfully", + self.id + ); + Ok(()) + } + + async fn consume( + &self, + topic_metadata: &TopicMetadata, + messages_metadata: MessagesMetadata, + messages: Vec, + ) -> Result<(), Error> { + let mut state = self.state.lock().await; + state.invocations_count += 1; + let invocation = state.invocations_count; + drop(state); + + info!( + "{CONNECTOR_NAME} with ID: {} received: {} messages, schema: {}, stream: {}, topic: {}, \ + partition: {}, offset: {}, invocation: {}", + self.id, + messages.len(), + messages_metadata.schema, + topic_metadata.stream, + topic_metadata.topic, + messages_metadata.partition_id, + messages_metadata.current_offset, + invocation + ); + + if self.circuit_breaker.is_open().await { + warn!( + "{CONNECTOR_NAME} connector with ID: {} — circuit breaker OPEN, refusing {} messages", + self.id, + messages.len() + ); + return Err(Error::CannotStoreData( + "Circuit breaker is open".to_string(), + )); + } + + let client = self.client.as_ref().ok_or_else(|| { + Error::Connection("client not initialized -- was open() called?".into()) + })?; + + let mut first_error: Option = None; + let mut written = 0u64; + let mut failed = 0u64; + + for batch in messages.chunks(self.batch_size_limit) { + match self.push_batch(client, batch).await { + Ok(()) => written += batch.len() as u64, + Err(err) => { + failed += batch.len() as u64; + error!( + "{CONNECTOR_NAME} connector with ID: {} failed a batch of {}: {err}", + self.id, + batch.len() + ); + if first_error.is_none() { + first_error = Some(err); + } + } + } + } + + // Record the circuit breaker outcome once per `consume()` call, not + // once per chunk — recording success partway through would reset + // the failure counter mid-consume and prevent the breaker from + // opening on a batch with sustained, mixed-success chunks. + match &first_error { + None => self.circuit_breaker.record_success(), + Some(e) if !matches!(e, Error::PermanentHttpError(_)) => { + self.circuit_breaker.record_failure().await; + } + Some(_) => {} + } + + let mut state = self.state.lock().await; + state.messages_written += written; + state.messages_failed += failed; + drop(state); + self.records_written_total + .fetch_add(written, Ordering::Relaxed); + + match first_error { + None => Ok(()), + Some(err) => Err(err), + } + } + + async fn close(&mut self) -> Result<(), Error> { + let state = self.state.lock().await; + info!( + "{CONNECTOR_NAME} connector with ID: {} closing. Stats: {} invocations, {} messages written, \ + {} messages failed", + self.id, state.invocations_count, state.messages_written, state.messages_failed + ); + drop(state); + self.client = None; + info!("{CONNECTOR_NAME} connector with ID: {} is closed.", self.id); + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn test_config() -> TemplateSinkConfig { + TemplateSinkConfig { + connection_string: SecretString::from("https://api.example.com"), + target: "events".to_string(), + auth_token: None, + health_check_path: None, + batch_size: Some(10), + timeout: Some("5s".to_string()), + max_retries: Some(2), + retry_delay: Some("10ms".to_string()), + retry_max_delay: Some("100ms".to_string()), + max_open_retries: Some(2), + open_retry_max_delay: Some("100ms".to_string()), + circuit_breaker_threshold: Some(3), + circuit_breaker_cool_down: Some("50ms".to_string()), + } + } + + #[tokio::test] + async fn open_rejects_empty_connection_string() { + let mut config = test_config(); + config.connection_string = SecretString::from(" "); + let mut sink = TemplateSink::new(1, config); + assert!(matches!( + sink.open().await, + Err(Error::InvalidConfigValue(_)) + )); + } + + #[tokio::test] + async fn open_rejects_invalid_target_identifier() { + let mut config = test_config(); + config.target = "events; DROP TABLE users;--".to_string(); + let mut sink = TemplateSink::new(1, config); + assert!(matches!( + sink.open().await, + Err(Error::InvalidConfigValue(_)) + )); + } + + #[test] + fn validate_identifier_accepts_plain_names() { + assert!(validate_identifier("target", "events_v2").is_ok()); + } + + #[test] + fn validate_identifier_rejects_empty() { + assert!(validate_identifier("target", "").is_err()); + } + + #[test] + fn validate_identifier_rejects_path_and_query_characters() { + for bad in [ + "../etc/passwd", + "events?x=1", + "events/../secrets", + "events;drop", + ] { + assert!( + validate_identifier("target", bad).is_err(), + "expected {bad:?} to be rejected" + ); + } + } + + #[tokio::test] + async fn consume_short_circuits_when_breaker_is_open() { + let sink = TemplateSink::new(1, test_config()); + sink.circuit_breaker.record_failure().await; + sink.circuit_breaker.record_failure().await; + sink.circuit_breaker.record_failure().await; + assert!(sink.circuit_breaker.is_open().await); + + let topic_metadata = TopicMetadata { + stream: "s".to_string(), + topic: "t".to_string(), + }; + let messages_metadata = MessagesMetadata { + partition_id: 1, + current_offset: 0, + schema: iggy_connector_sdk::Schema::Json, + }; + + let result = sink + .consume(&topic_metadata, messages_metadata, Vec::new()) + .await; + assert!(matches!(result, Err(Error::CannotStoreData(_)))); + } +} diff --git a/core/connectors/sources/README.md b/core/connectors/sources/README.md index a795735f43..640cd0ad5c 100644 --- a/core/connectors/sources/README.md +++ b/core/connectors/sources/README.md @@ -12,6 +12,7 @@ Source connectors are responsible for ingesting data from external sources into | **influxdb_source** | Polls InfluxDB with cursor-based timestamp tracking; supports V2 (Flux, annotated CSV) and V3 (SQL, JSONL) | | **postgres_source** | Reads rows from PostgreSQL tables with multiple strategies: delete after read, mark as processed, or timestamp tracking | | **random_source** | Generates random test messages (useful for testing and development) | +| **source_template** | Fill-in-the-blank starting point for a new source; framework/security plumbing done, two `TODO(Developer)` spots left | The source is represented by the single `Source` trait, which defines the basic interface for all source connectors. It provides methods for initializing the source, reading data from it, and closing the source. diff --git a/core/connectors/sources/source_template/Cargo.toml b/core/connectors/sources/source_template/Cargo.toml new file mode 100644 index 0000000000..a54d7cbccc --- /dev/null +++ b/core/connectors/sources/source_template/Cargo.toml @@ -0,0 +1,60 @@ +# 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. +# +# TEMPLATE — rename the package (and this directory) to +# `iggy_connector__source` before publishing, and update the +# `[[sources]]` entry you add to the workspace root Cargo.toml accordingly. + +[package] +name = "iggy_connector_template_source" +version = "0.1.0" +description = "Template for an Apache Iggy source connector — copy this crate and fill in the TODO sections." +edition = "2024" +license = "Apache-2.0" +keywords = ["iggy", "messaging", "streaming"] +categories = ["command-line-utilities", "database", "network-programming"] +homepage = "https://iggy.apache.org" +documentation = "https://iggy.apache.org/docs" +repository = "https://github.com/apache/iggy" +publish = false + +[package.metadata.cargo-machete] +# dashmap is used only inside the `source_connector!` macro expansion, so a +# naive unused-dependency scan won't see the usage — keep it ignored rather +# than removing it, or the plugin will fail to compile. +ignored = ["dashmap"] + +[lib] +crate-type = ["cdylib", "lib"] + +[dependencies] +async-trait = { workspace = true } +dashmap = { workspace = true } +humantime = { workspace = true } +iggy_common = { workspace = true } +iggy_connector_sdk = { workspace = true } +reqwest = { workspace = true } +reqwest-middleware = { workspace = true } +rmp-serde = { workspace = true } +secrecy = { workspace = true } +serde = { workspace = true } +serde_json = { workspace = true } +tokio = { workspace = true } +tracing = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true, features = ["test-util"] } diff --git a/core/connectors/sources/source_template/README.md b/core/connectors/sources/source_template/README.md new file mode 100644 index 0000000000..f4a39d3bac --- /dev/null +++ b/core/connectors/sources/source_template/README.md @@ -0,0 +1,64 @@ +# Template source connector + +Starting point for a new Apache Iggy **source** connector. Everything except +talking to your actual external system is already implemented and follows +the project's required resilience/security patterns — see the module-level +doc comment at the top of `src/lib.rs` for the full rationale, and the +`iggy-connector-review` skill / the "Building Connectors That Pass Review" +blog post for the checklist this template is built against. + +## What's already done for you + +- Config parsing with `#[serde(deny_unknown_fields)]` so a typo in a TOML + file fails loudly instead of silently doing nothing. +- Config validation in `open()` (not `new()`, which has no way to return an + error). +- `connection_string` and the optional `auth_token` field both typed as + `SecretString`, since either can carry credentials. +- A retry-wrapped HTTP client (`iggy_connector_sdk::retry::build_retry_client`) + and a startup connectivity probe with its own backoff + (`check_connectivity_with_retry`). +- A `CircuitBreaker` that's actually consulted before polling and updated + after every attempt — not just constructed and forgotten. +- Cursor staging: `poll()` never commits its progress directly; it stages a + candidate and `on_batch_result()` commits it only on `Ack`, discarding it + on `Nack` so a failed delivery gets re-polled instead of silently lost. +- The `source_connector!` FFI macro invocation and a `Cargo.toml` with the + right `crate-type`, workspace-pinned dependencies, and license header. +- Tests for config validation and the Ack/Nack state-commit behavior. + +## What you need to fill in + +Search for `TODO(Developer)` in `src/lib.rs` — there are exactly two spots: + +1. **`build_raw_client()`** — if your source isn't HTTP, replace the + `reqwest::Client` construction with your driver's connection/pool setup + (see `core/connectors/sources/postgres_source` for a real non-HTTP + example), store it on `TemplateSource` (you'll need to add a field — + `client: Option` here is HTTP-specific), and adjust + or remove the `check_connectivity_with_retry` call in `open()` in favor + of whatever connectivity check your driver offers. +2. **`fetch_records()`** — fetch up to `self.batch_size` new records from + your system, ordered after `cursor` (`None` = start from the beginning, + or from "now" — whichever fits your source). Map each result to a + `FetchedRecord { cursor_value, payload }`, using something monotonically + increasing as `cursor_value` (a timestamp, an ID, a page token) — that's + what lets the cursor-staging logic advance correctly. + +## Using it + +1. Copy this directory, rename it and the package in `Cargo.toml` + (`iggy_connector__source`), and add it to the `members` list in + the workspace root `Cargo.toml`. +2. Fill in the two `TODO(Developer)` sections. +3. Update `config.toml` with your real `connection_string` and any + settings specific to your system; delete `auth_token` if you don't need + it, or add fields of your own the same way (see `TemplateSourceConfig`). +4. `cargo build --release -p iggy_connector__source`, point a + runtime connector config file's `path` at the built `.so`/`.dylib`/`.dll`, + and run the connector runtime — see `core/connectors/README.md` in this + repo for the full runtime quick-start. +5. Before opening a PR: `cargo test`, `cargo clippy --all-targets`, + `cargo fmt --check`, and re-read the connector-review checklist once more + with fresh eyes — most review round-trips come from one of the items in + that list, not from the connector-specific logic in `fetch_records()`. diff --git a/core/connectors/sources/source_template/config.toml b/core/connectors/sources/source_template/config.toml new file mode 100644 index 0000000000..53ac14874e --- /dev/null +++ b/core/connectors/sources/source_template/config.toml @@ -0,0 +1,47 @@ +# 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. + +type = "source" +key = "template" +enabled = true +version = 0 +name = "Template source" +path = "../../target/release/libiggy_connector_template_source" +plugin_config_format = "toml" +verbose = false + +[[streams]] +stream = "example_stream" +topic = "example_topic" +schema = "json" +batch_length = 100 +linger_time = "5ms" + +[plugin_config] +connection_string = "https://api.example.com" +health_check_path = "/health" +batch_size = 100 +poll_interval = "1s" +timeout = "30s" +max_retries = 3 +retry_delay = "500ms" +retry_max_delay = "5s" +max_open_retries = 10 +open_retry_max_delay = "60s" +circuit_breaker_threshold = 5 +circuit_breaker_cool_down = "30s" +# auth_token = "replace-me" # uncomment if your source needs bearer/API-key auth diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs new file mode 100644 index 0000000000..24f52b44c1 --- /dev/null +++ b/core/connectors/sources/source_template/src/lib.rs @@ -0,0 +1,586 @@ +// 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. + +//! Template Apache Iggy **source** connector. +//! +//! Everything in this file is already wired up and follows the patterns the +//! connector-review checklist expects: config validation happens in +//! `open()`, secrets are `SecretString`, outbound calls go through a +//! retry-wrapped client plus a circuit breaker, and the read cursor is +//! staged in `poll()` and only committed in `on_batch_result()` on an ACK so +//! a dropped/nacked batch can be re-polled instead of silently lost. +//! +//! There are exactly **two** places you need to touch, each marked +//! `TODO(Developer)`: +//! 1. `TemplateSource::connect()` — build your actual client/connection +//! from `config.connection_string` (and `config.auth_token`, if used). +//! 2. `TemplateSource::fetch_records()` — fetch up to `batch_size` new +//! records from your external system, starting after `cursor`. +//! +//! This template assumes an HTTP-ish source and uses `reqwest` wrapped by +//! the SDK's retry middleware, because that's what `iggy_connector_sdk::retry` +//! is built for and it covers the common case. If your source talks to +//! something else (a database, a queue, a filesystem), swap the client type +//! in `connect()`/`fetch_records()` for your driver of choice and lean on +//! its own retry/pooling behavior — keep the surrounding shape (validation +//! in `open()`, circuit breaker, cursor staging, batching) unchanged. See +//! `core/connectors/sources/postgres_source` in this repo for a real +//! non-HTTP example of that same shape. + +use async_trait::async_trait; +use iggy_connector_sdk::retry::{ + CircuitBreaker, ConnectivityConfig, build_retry_client, check_connectivity_with_retry, + parse_duration, +}; +use iggy_connector_sdk::{ + ConnectorState, Error, ProducedMessage, ProducedMessages, Schema, Source, + source::SourceBatchResult, source_connector, +}; +use reqwest::Url; +use reqwest_middleware::ClientWithMiddleware; +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize}; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Duration; +use tokio::sync::Mutex; +use tracing::{error, info, warn}; + +source_connector!(TemplateSource); + +const CONNECTOR_NAME: &str = "Template source"; + +const DEFAULT_POLL_INTERVAL: &str = "1s"; +const DEFAULT_BATCH_SIZE: u32 = 100; +const DEFAULT_TIMEOUT: &str = "30s"; +const DEFAULT_MAX_RETRIES: u32 = 3; +const DEFAULT_RETRY_DELAY: &str = "500ms"; +const DEFAULT_RETRY_MAX_DELAY: &str = "5s"; +const DEFAULT_MAX_OPEN_RETRIES: u32 = 10; +const DEFAULT_OPEN_RETRY_MAX_DELAY: &str = "60s"; +const DEFAULT_CIRCUIT_BREAKER_THRESHOLD: u32 = 5; +const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; + +// ── Configuration ─────────────────────────────────────────────────────────── +// +// Every tunable except `connection_string` is optional with a sane default, +// and unknown keys are rejected outright so a typo in a TOML file fails at +// load time instead of silently doing nothing. + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct TemplateSourceConfig { + /// TODO(Developer): document the exact shape this connector expects, e.g. + /// "https://api.example.com" or "postgres://user:pass@host:5432/db". + /// `SecretString` because DSNs commonly embed credentials — never plain + /// `String` for this field, see `PostgresSinkConfig::connection_string` + /// in `sinks/postgres_sink` for the same pattern. + #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] + pub connection_string: SecretString, + + /// Example of a secret-shaped setting. `SecretString` keeps it out of + /// `Debug`/log output; delete this field if `connection_string` already + /// carries all required auth. Read it with `.expose_secret()` (from the + /// `secrecy::ExposeSecret` trait) at the one place you actually need the + /// plaintext — e.g. when building an auth header in `connect()`. + #[serde( + default, + serialize_with = "iggy_common::serde_secret::serialize_optional_secret" + )] + pub auth_token: Option, + + /// Optional path (e.g. "/health") probed with retry during `open()` + /// before the connector is considered ready. Leave unset if your target + /// has no health endpoint — the probe is skipped, not failed, in that case. + pub health_check_path: Option, + + pub batch_size: Option, + pub poll_interval: Option, + pub timeout: Option, + pub max_retries: Option, + pub retry_delay: Option, + pub retry_max_delay: Option, + pub max_open_retries: Option, + pub open_retry_max_delay: Option, + pub circuit_breaker_threshold: Option, + pub circuit_breaker_cool_down: Option, +} + +// ── Internal state ────────────────────────────────────────────────────────── + +/// Read cursor. Kept as a plain `Option` so it fits whatever ordering +/// field your source uses (a timestamp, an auto-increment ID, an opaque +/// pagination token, ...) — stringify it however makes sense for your data. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +struct State { + cursor: Option, +} + +/// One record as fetched from the external system, before it's turned into +/// an Iggy message. Replace the `payload` type with whatever your +/// `fetch_records()` actually produces. +struct FetchedRecord { + /// The value `cursor` should advance to once this record's batch is + /// acknowledged — typically this record's timestamp/ID/token. + cursor_value: String, + payload: serde_json::Value, +} + +#[derive(Debug)] +pub struct TemplateSource { + id: u32, + config: TemplateSourceConfig, + client: Option, + circuit_breaker: Arc, + batch_size: u32, + poll_interval: Duration, + retry_delay: Duration, + state: Mutex, + pending_state: Mutex>, + records_produced: AtomicU64, +} + +impl TemplateSource { + pub fn new(id: u32, config: TemplateSourceConfig, state: Option) -> Self { + let poll_interval = *humantime::Duration::from_str_lossy( + config + .poll_interval + .as_deref() + .unwrap_or(DEFAULT_POLL_INTERVAL), + ); + let retry_delay = parse_duration(config.retry_delay.as_deref(), DEFAULT_RETRY_DELAY); + let circuit_breaker = Arc::new(CircuitBreaker::new( + config + .circuit_breaker_threshold + .unwrap_or(DEFAULT_CIRCUIT_BREAKER_THRESHOLD), + parse_duration( + config.circuit_breaker_cool_down.as_deref(), + DEFAULT_CIRCUIT_BREAKER_COOL_DOWN, + ), + )); + let batch_size = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE).max(1); + + let restored_state = state + .and_then(|s| s.deserialize::(CONNECTOR_NAME, id)) + .inspect(|s| { + info!( + "Restored state for {CONNECTOR_NAME} connector with ID: {id}. Cursor: {:?}", + s.cursor + ); + }); + + Self { + id, + config, + client: None, + circuit_breaker, + batch_size, + poll_interval, + retry_delay, + state: Mutex::new(restored_state.unwrap_or_default()), + pending_state: Mutex::new(None), + records_produced: AtomicU64::new(0), + } + } + + fn serialize_state(&self, state: &State) -> Option { + ConnectorState::serialize(state, CONNECTOR_NAME, self.id) + } + + /// TODO(Developer): build your actual client/connection here using + /// `self.config.connection_string` (and `self.config.auth_token`, if + /// your system needs it). This template builds a plain `reqwest::Client` + /// to hand to `build_retry_client` — if you're not talking HTTP, replace + /// this with e.g. a `sqlx::PgPool::connect(...)` or your driver's + /// equivalent, store it on `self` (add a field, since `client` here is + /// HTTP-specific), and skip the `check_connectivity_with_retry` call + /// below in favor of whatever your driver offers (a ping, a test query). + fn build_raw_client(&self) -> Result { + let timeout = parse_duration(self.config.timeout.as_deref(), DEFAULT_TIMEOUT); + reqwest::Client::builder() + .timeout(timeout) + .build() + .map_err(|e| Error::Connection(format!("failed to build HTTP client: {e}"))) + } + + /// TODO(Developer): fetch up to `self.batch_size` new records from your + /// external system, ordered after `cursor` (`None` means "from the + /// beginning" or "from now" — whichever is right for your source). + /// Use `self.config.connection_string` as the base address and + /// `self.client` (already retry-wrapped) to make the request. Map each + /// result row/document/event to a `FetchedRecord`, using something that + /// monotonically increases (a timestamp, an ID, a page token) as + /// `cursor_value` so the state-staging logic in `poll()` below can + /// advance the cursor correctly. + async fn fetch_records( + &self, + client: &ClientWithMiddleware, + cursor: Option<&str>, + ) -> Result, Error> { + let _ = (client, cursor); // remove once implemented + Err(Error::InitError( + "TemplateSource::fetch_records is not implemented yet — see the TODO(Developer) comment \ + in template_source/src/lib.rs" + .to_string(), + )) + } +} + +// ── Source trait ──────────────────────────────────────────────────────────── + +#[async_trait] +impl Source for TemplateSource { + async fn open(&mut self) -> Result<(), Error> { + // Structural validation happens here, not in `new()`, because only + // `open()` can return an error — `new()` is a plain factory function + // with nowhere to send a "this config is invalid" result. + if self + .config + .connection_string + .expose_secret() + .trim() + .is_empty() + { + return Err(Error::InvalidConfigValue( + "connection_string must not be empty".to_string(), + )); + } + + info!( + "Opening {CONNECTOR_NAME} connector with ID: {}, batch_size: {}, poll_interval: {:?}", + self.id, self.batch_size, self.poll_interval + ); + + let raw_client = self.build_raw_client()?; + + if let Some(health_path) = &self.config.health_check_path { + let base = Url::parse(self.config.connection_string.expose_secret()).map_err(|e| { + Error::InvalidConfigValue(format!("connection_string is not a valid URL: {e}")) + })?; + let health_url = base.join(health_path).map_err(|e| { + Error::InvalidConfigValue(format!("invalid health_check_path: {e}")) + })?; + check_connectivity_with_retry( + &raw_client, + health_url, + CONNECTOR_NAME, + self.id, + &ConnectivityConfig { + max_open_retries: self + .config + .max_open_retries + .unwrap_or(DEFAULT_MAX_OPEN_RETRIES), + open_retry_max_delay: parse_duration( + self.config.open_retry_max_delay.as_deref(), + DEFAULT_OPEN_RETRY_MAX_DELAY, + ), + retry_delay: self.retry_delay, + }, + ) + .await?; + } else { + warn!( + "{CONNECTOR_NAME} connector with ID: {} has no health_check_path configured — \ + skipping the startup connectivity probe. Consider adding one.", + self.id + ); + } + + self.client = Some(build_retry_client( + raw_client, + self.config + .max_retries + .unwrap_or(DEFAULT_MAX_RETRIES) + .max(1), + self.retry_delay, + parse_duration( + self.config.retry_max_delay.as_deref(), + DEFAULT_RETRY_MAX_DELAY, + ), + CONNECTOR_NAME, + )); + + info!( + "{CONNECTOR_NAME} connector with ID: {} opened successfully", + self.id + ); + Ok(()) + } + + async fn poll(&self) -> Result { + // If the breaker is open, sleep for the normal poll interval and + // return an empty (not an error) result. Returning `Err` here would + // make the runtime retry `poll()` again immediately with no delay — + // see `handle_messages` in the SDK's source container — so an empty + // ACK-free result is how a source waits out a known-bad window + // without busy-looping or counting against the NACK budget. + if self.circuit_breaker.is_open().await { + warn!( + "{CONNECTOR_NAME} connector with ID: {} — circuit breaker OPEN, skipping poll", + self.id + ); + tokio::time::sleep(self.poll_interval).await; + return Ok(ProducedMessages { + schema: Schema::Json, + messages: Vec::new(), + state: None, + }); + } + tokio::time::sleep(self.poll_interval).await; + + let client = self.client.as_ref().ok_or_else(|| { + Error::Connection("client not initialized -- was open() called?".into()) + })?; + let cursor = self.state.lock().await.cursor.clone(); + + let records = match self.fetch_records(client, cursor.as_deref()).await { + Ok(records) => { + self.circuit_breaker.record_success(); + records + } + Err(err) => { + if !matches!(err, Error::PermanentHttpError(_)) { + self.circuit_breaker.record_failure().await; + } + return Err(err); + } + }; + + if records.is_empty() { + return Ok(ProducedMessages { + schema: Schema::Json, + messages: Vec::new(), + state: None, + }); + } + + let mut messages = Vec::with_capacity(records.len()); + let mut new_cursor = cursor; + for record in records { + new_cursor = Some(record.cursor_value); + let Ok(payload) = serde_json::to_vec(&record.payload) else { + error!( + "Failed to serialize a record fetched by {CONNECTOR_NAME} connector with ID: {}", + self.id + ); + continue; + }; + messages.push(ProducedMessage { + id: None, + headers: None, + checksum: None, + timestamp: None, + origin_timestamp: None, + payload, + }); + } + + let candidate_state = State { cursor: new_cursor }; + let persisted_state = self.serialize_state(&candidate_state).ok_or_else(|| { + Error::Serialization(format!( + "failed to serialize state for {CONNECTOR_NAME} connector with ID: {}", + self.id + )) + })?; + *self.pending_state.lock().await = Some(candidate_state); + + self.records_produced + .fetch_add(messages.len() as u64, Ordering::Relaxed); + + Ok(ProducedMessages { + schema: Schema::Json, + messages, + state: Some(persisted_state), + }) + } + + /// The staged cursor from `poll()` is only committed here, and only on + /// an ACK. A NACK (delivery failed, batch timed out, runtime is + /// shutting down) discards the candidate so the same range is re-polled + /// next time — the cursor never moves past data that wasn't confirmed + /// delivered. If `fetch_records()` ever needs to perform a destructive + /// read against the source (delete-after-read, mark-as-processed), + /// stage that side effect the same way and only apply it here on `Ack`. + async fn on_batch_result(&self, result: SourceBatchResult) -> Result<(), Error> { + let candidate_state = self.pending_state.lock().await.take(); + if result == SourceBatchResult::Ack + && let Some(candidate_state) = candidate_state + { + *self.state.lock().await = candidate_state; + } + Ok(()) + } + + async fn close(&mut self) -> Result<(), Error> { + let state = self.state.lock().await; + info!( + "{CONNECTOR_NAME} connector with ID: {} closed. Cursor: {:?}, total records produced: {}", + self.id, + state.cursor, + self.records_produced.load(Ordering::Relaxed) + ); + drop(state); + self.client = None; + Ok(()) + } +} + +// Small local shim so `new()` doesn't need to pull in `humantime::Duration`'s +// `FromStr` (which returns `Result`) just to apply a default — mirrors the +// fallback-with-warning behavior of `iggy_connector_sdk::retry::parse_duration` +// for the one duration field (`poll_interval`) that isn't itself optional in +// spirit (there's always a poll interval, just maybe the default one). +trait DurationExt { + fn from_str_lossy(s: &str) -> humantime::Duration; +} +impl DurationExt for humantime::Duration { + fn from_str_lossy(s: &str) -> humantime::Duration { + use std::str::FromStr; + humantime::Duration::from_str(s).unwrap_or_else(|_| { + warn!("Invalid poll_interval {s:?}, falling back to {DEFAULT_POLL_INTERVAL}"); + humantime::Duration::from_str(DEFAULT_POLL_INTERVAL) + .expect("DEFAULT_POLL_INTERVAL must itself be a valid duration literal") + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn test_config() -> TemplateSourceConfig { + TemplateSourceConfig { + connection_string: SecretString::from("https://api.example.com"), + auth_token: None, + health_check_path: None, + batch_size: Some(50), + poll_interval: Some("50ms".to_string()), + timeout: Some("5s".to_string()), + max_retries: Some(2), + retry_delay: Some("10ms".to_string()), + retry_max_delay: Some("100ms".to_string()), + max_open_retries: Some(2), + open_retry_max_delay: Some("100ms".to_string()), + circuit_breaker_threshold: Some(3), + circuit_breaker_cool_down: Some("50ms".to_string()), + } + } + + #[tokio::test] + async fn open_rejects_empty_connection_string() { + let mut config = test_config(); + config.connection_string = SecretString::from(" "); + let mut source = TemplateSource::new(1, config, None); + let result = source.open().await; + assert!(matches!(result, Err(Error::InvalidConfigValue(_)))); + } + + #[tokio::test] + async fn given_no_state_should_start_with_no_cursor() { + let source = TemplateSource::new(1, test_config(), None); + assert_eq!(source.state.lock().await.cursor, None); + } + + #[tokio::test] + async fn given_persisted_state_should_restore_cursor() { + let state = State { + cursor: Some("2024-01-01T00:00:00Z".to_string()), + }; + let serialized = rmp_serde::to_vec(&state).expect("failed to serialize state"); + let source = TemplateSource::new(1, test_config(), Some(ConnectorState(serialized))); + assert_eq!( + source.state.lock().await.cursor, + Some("2024-01-01T00:00:00Z".to_string()) + ); + } + + #[tokio::test] + async fn given_invalid_persisted_state_should_start_fresh() { + let source = TemplateSource::new( + 1, + test_config(), + Some(ConnectorState(b"not valid msgpack".to_vec())), + ); + assert_eq!(source.state.lock().await.cursor, None); + } + + #[test] + fn state_should_be_serializable_and_deserializable() { + let original = State { + cursor: Some("2024-01-01T00:00:00Z".to_string()), + }; + + let serialized = rmp_serde::to_vec(&original).expect("failed to serialize state"); + let deserialized: State = + rmp_serde::from_slice(&serialized).expect("failed to deserialize state"); + + assert_eq!(original.cursor, deserialized.cursor); + } + + #[tokio::test] + async fn given_ack_should_commit_staged_cursor() { + let source = TemplateSource::new(1, test_config(), None); + *source.pending_state.lock().await = Some(State { + cursor: Some("next-cursor".to_string()), + }); + + source + .on_batch_result(SourceBatchResult::Ack) + .await + .expect("ACK should be applied"); + + assert_eq!( + source.state.lock().await.cursor, + Some("next-cursor".to_string()) + ); + assert!(source.pending_state.lock().await.is_none()); + } + + #[tokio::test] + async fn given_nack_should_discard_staged_cursor() { + let source = TemplateSource::new(1, test_config(), None); + *source.pending_state.lock().await = Some(State { + cursor: Some("next-cursor".to_string()), + }); + + source + .on_batch_result(SourceBatchResult::Nack) + .await + .expect("NACK should be applied"); + + // The committed cursor is unchanged (still None) — the candidate is + // simply discarded so the same range is polled again. + assert_eq!(source.state.lock().await.cursor, None); + assert!(source.pending_state.lock().await.is_none()); + } + + #[tokio::test] + async fn poll_returns_empty_without_error_when_circuit_is_open() { + let source = TemplateSource::new(1, test_config(), None); + source.circuit_breaker.record_failure().await; + source.circuit_breaker.record_failure().await; + source.circuit_breaker.record_failure().await; + assert!(source.circuit_breaker.is_open().await); + + let result = source + .poll() + .await + .expect("open breaker should not error poll()"); + assert!(result.messages.is_empty()); + assert!(result.state.is_none()); + } +} From 8f097c8525038ca01003e82ba3e62ecedb5cb18c Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Tue, 25 Aug 2026 09:16:11 +0530 Subject: [PATCH 02/10] Normalising the naming convention --- .claude/skills/connector-pr-review/SKILL.md | 248 +++++++++ .claude/skills/connector-runtime/SKILL.md | 4 +- .claude/skills/connector-sink/SKILL.md | 3 +- .claude/skills/connector-sink/TEMPLATE.md | 329 ++++++++++-- .claude/skills/connector-source/SKILL.md | 104 +++- .claude/skills/connector-source/TEMPLATE.md | 487 +++++++++++++++--- .claude/skills/connectors-overview/SKILL.md | 70 ++- core/connectors/BLOG_POST.md | 4 +- core/connectors/sinks/README.md | 2 +- core/connectors/sinks/sink_template/README.md | 4 +- .../connectors/sinks/sink_template/src/lib.rs | 10 +- core/connectors/sources/README.md | 2 +- .../sources/source_template/README.md | 4 +- .../sources/source_template/src/lib.rs | 10 +- 14 files changed, 1078 insertions(+), 203 deletions(-) create mode 100644 .claude/skills/connector-pr-review/SKILL.md diff --git a/.claude/skills/connector-pr-review/SKILL.md b/.claude/skills/connector-pr-review/SKILL.md new file mode 100644 index 0000000000..c386099c79 --- /dev/null +++ b/.claude/skills/connector-pr-review/SKILL.md @@ -0,0 +1,248 @@ +--- +name: connector-pr-review +description: Review checklist for Apache Iggy connector sink/source PRs. Load when reviewing a connectors plugin PR, when authoring a new sink/source and wanting to pre-flight against common review blockers, or when diagnosing why a connectors PR is stuck in review. Encodes recurring review patterns mined from real apache/iggy connector PRs. NOT for runtime/SDK internals (use connector-runtime / connector-sdk). +--- + +# Connector PR review checklist + +> Universal rules live in [connectors-overview](../connectors-overview/SKILL.md). +> Authoring skills: [connector-sink](../connector-sink/SKILL.md), +> [connector-source](../connector-source/SKILL.md), +> [connector-testing](../connector-testing/SKILL.md). +> Fill-in-the-blank kits: those skills' `TEMPLATE.md` files. + +Use this skill to **catch the issues that repeatedly burn review cycles** +before asking for a human re-review. Cite symbols/paths, not stale line numbers. + +## Contents + +- [How to use](#how-to-use) +- [Blockers (must fix before merge)](#blockers-must-fix-before-merge) +- [High-frequency convention nits](#high-frequency-convention-nits) +- [Delivery semantics (document honestly)](#delivery-semantics-document-honestly) +- [PR / CI hygiene](#pr--ci-hygiene) +- [Pre-flight author checklist](#pre-flight-author-checklist) +- [Evidence base](#evidence-base) + +## How to use + +1. Load this skill for any PR under `core/connectors/sinks/` or `core/connectors/sources/`. +2. Walk **Blockers** first. Any hit is CHANGES_REQUESTED. +3. Then **Convention nits** and **Delivery semantics**. +4. End with **PR / CI hygiene** (cheap passes that still delay first review). +5. Prefer "copy the closest exemplar" over inventing new knobs. + +## Blockers (must fix before merge) + +### B1. Secrets + +- [ ] Every credential field is `secrecy::SecretString` with + `#[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")]`. +- [ ] No connection string / API key / token in `info!`/`debug!`/`error!` / + `format!` into SQL / file-state metadata. +- [ ] Source state must not persist URL userinfo or bearer tokens. + +Plain `String` for a credential is a review-blocker. Pattern: +`PostgresSinkConfig::connection_string` in `sinks/postgres_sink`. + +### B2. Swallowing `Err` while offsets advance (sinks) + +- [ ] `consume()` must **not** catch a batch failure and return `Ok(())`. +- [ ] Prefer `last_err` pattern: process all batches, return the last transient + error (see `connector-sink` Hard rules / TEMPLATE). +- [ ] README must not claim "no data loss" / strong idempotency unless the + backend + runtime path actually enforce it. + +Today the runtime can commit consumer offsets even when plugin errors are +poorly surfaced (#2927 / #2928 class issues). Authors must be honest about +loss windows instead of overselling. + +### B3. Source cursor / side-effects staged for `on_batch_result`, not committed in `poll()` + +Since #3855, this is an SDK-enforced contract, not just a convention: +`poll()` returns *candidate* state; the runtime sends the batch, saves +that state on success, then calls `on_batch_result(Ack | Nack)`. Only +`on_batch_result` may commit. + +- [ ] `poll()` stages cursor changes and destructive work (delete-after-read / + mark-processed / ACK upstream) - it does not mutate committed state or + touch upstream rows directly. Committing in `poll()` leaves nothing to + roll back on a Nack, defeating the point of the handshake. +- [ ] `on_batch_result(SourceBatchResult::Ack)` commits the staged work; + `on_batch_result(SourceBatchResult::Nack)` discards it so the batch is + redelivered against unchanged committed state. A source with no staged + work (e.g. a pure generator) may rely on the SDK's default no-op impl. +- [ ] Returning `Err` from `on_batch_result` is deliberate - it stops the SDK + from polling further rather than risk silently advancing past a failed + rollback. Don't swallow a rollback failure into `Ok(())`. +- [ ] Always return `ConnectorState` in every `ProducedMessages` that made + progress; return `state: None` for an empty poll that made none (avoids + an unnecessary write and can't persist state left over from a failed + batch). +- [ ] `poll()` sleeps **first**, then fetches (never sleep after holding a batch). + +### B4. Transient vs permanent errors + +- [ ] Infra/auth/schema-gone failures map to `Error::PermanentHttpError` / + `Error::InitError` / `Error::SchemaMismatch` — not `InvalidRecord`. +- [ ] Retryable network/5xx/SQLSTATE map to transient variants + (`HttpRequestFailed`, `Connection`, `CannotStoreData`). +- [ ] Do **not** classify retryability by substring-matching `err.to_string()`. +- [ ] `max_retries` means **total attempts** (default 3). README must match code. +- [ ] Cap retry budget so a dead backend cannot delay shutdown unboundedly. + +### B5. Idempotency claims must be real + +- [ ] Stable `ProducedMessage.id` / sink dedup key from natural IDs + (table+PK, document `_id`, `stream:topic:partition:message_id`) — + never random UUIDs per emit. +- [ ] If the backend PK / unique index is informational only (e.g. Redshift), + do not advertise idempotency in README. +- [ ] External workflow IDs (Airflow `dag_run_id`, etc.) must be deterministic + across retries. + +### B6. Secrets / license policy for new SDKs + +- [ ] New backend crates pass `scripts/ci/third-party-licenses.sh` (no BUSL / + incompatible licenses pulled into the tree). +- [ ] Prefer workspace deps; avoid vendoring a license-hostile SDK just to wrap HTTP. + +### B7. Tests that must exist + +#### Sources + +- [ ] Four canonical state tests (restore / no-state / invalid-state / + round-trip), plus two ACK/NACK tests (`given_ack_when_batch_is_staged_ + should_commit_candidate_state`, `given_nack_when_batch_is_staged_ + should_keep_committed_state`) if `on_batch_result` is overridden - six + total. Copy `sources/random_source/src/lib.rs::tests`. A source relying + on the SDK's default no-op `on_batch_result` (no staged work) may skip + the ACK/NACK pair. + +#### Any external backend plugin + +- [ ] At least one real-infra integration test under + `core/integration/tests/connectors//` with `#[iggy_harness]` + + `testcontainers-modules` (or `wiremock` for pure HTTP). +- [ ] No false-green mocks that diverge from real backend semantics + (Decimal, COPY, PK enforcement, etc.). + +### B8. Config validation timing + +- [ ] Structural validation + unknown enum rejection in `new()` / + `open()` — not on first `poll()`/`consume()` after sleep. +- [ ] Connectivity check in `open()`; fail with `Error::InitError`. +- [ ] Invalid restored state: start fresh + `warn!`, but do **not** silently + re-emit an entire index without calling that out in README. +- [ ] Config flag combos that no-op should `warn!` or `Err`, not silently ignore. + +## High-frequency convention nits + +These are "cheap" but burn full review rounds when missed. + +### C1. Copy the closest exemplar + +- [ ] File layout, log labels, error mapping, and test structure match the + nearest sibling (`postgres_*`, `http_sink`, `elasticsearch_*`, …). +- [ ] Do not invent new names for existing knobs. + +### C2. Config knob name canon + +| Concept | Canonical field | Notes | +| ------- | --------------- | ----- | +| Request timeout | `timeout` | Not `request_timeout` | +| Retry attempts | `max_retries` | Total attempts, default 3 | +| Base backoff | `retry_delay` | humantime `Option` | +| Backoff ceiling | `max_retry_delay` | Not `retry_max_delay` | +| Poll cadence (sources) | `poll_interval` | humantime; sleep first | +| Plugin verbosity | `verbose_logging` | Mirror runtime `verbose` | +| Credentials | `connection_string` / `api_key` / … | Always `SecretString` | + +- [ ] Durations: `Option` + `humantime::Duration` in `new()`; fall back + with `warn!`, never panic. Workspace `humantime` — not a pinned + `humantime-serde`. +- [ ] New fields: `Option` + `#[serde(default)]` where needed. +- [ ] Prefer `#[serde(deny_unknown_fields)]` on plugin config so typo’d knobs + fail loud. + +### C3. Crate / path / docs checklist + +- [ ] `[lib] crate-type = ["cdylib", "lib"]`. +- [ ] Example TOML plugin path uses `../../target/release/lib…` like siblings. +- [ ] Row added to `sinks/README.md` or `sources/README.md`. +- [ ] Sample under `runtime/example_config/connectors/`. +- [ ] README defaults **byte-equal** to consts in code (diff them). +- [ ] No links to non-existent docs. + +### C4. Hot path + +- [ ] No `payload.clone().try_to_bytes()` — use `try_to_bytes(&self)`. +- [ ] `Vec::with_capacity(n)` for per-batch buffers. +- [ ] No `std::sync::Mutex` across `.await`; use `tokio::sync::Mutex`. +- [ ] `&self` on `consume` / `poll` (interior mutability only). +- [ ] No `tokio::spawn` inside plugin code. +- [ ] No `unwrap()`/`expect()` on external I/O outside tests. +- [ ] No eager `format!` around tracing args. + +### C5. Containers / fixtures + +- [ ] Testcontainers named `iggy-test-*` via `fixtures::unique_container_name` + (or fixed `iggy-test-` for reuse fixtures). +- [ ] Custom Docker networks cleaned up. + +## Delivery semantics (document honestly) + +Every new connector README must answer in one short paragraph: + +1. **What happens on transient failure?** (retry N times, then Err) +2. **What happens on permanent failure?** (drop/skip vs fail batch) +3. **What is the duplication window?** (at-least-once because state saves + after Iggy send; or at-most-once if upstream ACK precedes send — say so) +4. **What is the dedup key?** (or "none — duplicates possible") + +If the answer is hand-wavy, the PR is not ready. + +## PR / CI hygiene + +- [ ] Conventional commit: `feat(connectors): …` / `fix(connectors): …`. +- [ ] PR template filled (motivation, linked issue). +- [ ] `cargo fmt --all` + `cargo sort --no-format --workspace` + + `cargo clippy -p --all-targets -- -D warnings` + + `cargo test -p ` green locally. +- [ ] Minimal `Cargo.lock` delta — no unrelated dependency churn. +- [ ] Do not modify unrelated Java/Python/foreign SDK trees in a connectors PR. +- [ ] Mark ready for review only after the above; stale-bot closes waiting PRs. + +## Pre-flight author checklist + +Paste into the PR description (or run mentally before `/ready`): + +```text +[ ] SecretString on all credentials; no secret logs/state +[ ] consume/poll never returns Ok(()) after a failed batch that should retry +[ ] Transient vs permanent errors mapped (no Display substring matching) +[ ] Stable message / dedup IDs (no random UUID per emit) +[ ] README delivery semantics paragraph present and honest +[ ] README defaults match code consts +[ ] deny_unknown_fields on plugin config +[ ] Canonical knob names (timeout, max_retries, retry_delay, poll_interval) +[ ] Sources: 4 state tests (+2 ACK/NACK if on_batch_result overridden); sleep-first poll; state staged in poll, committed only in on_batch_result +[ ] External backend: real-infra integration test (not a lying mock) +[ ] example_config + sinks/sources README row +[ ] fmt / sort --no-format / clippy -D warnings / unit tests green +[ ] Cargo.lock churn limited to this crate's deps +``` + +## Evidence base + +Recurring comments mined from connector PRs including (non-exhaustive): +SurrealDB sink (#3453), Meilisearch sink/source (#3497/#3498), OpenSearch +source (#3515), Quickwit convention (#3523), MySQL source (#3568), JDBC +source (#3588), Doris retry (#3574), Redshift sink (#3654), Airflow trigger +(#3716), Fluss sink (#3782). Highest-density themes: delivery/offset +semantics, idempotency IDs, transient/permanent mapping, config-name drift, +secrets, README/code drift, false-green tests, CI/lockfile hygiene. + +--- + +Discussion / help: see [AGENTS.md](../../../AGENTS.md#discussion-and-support). diff --git a/.claude/skills/connector-runtime/SKILL.md b/.claude/skills/connector-runtime/SKILL.md index cee028180d..ce194c08e6 100644 --- a/.claude/skills/connector-runtime/SKILL.md +++ b/.claude/skills/connector-runtime/SKILL.md @@ -211,7 +211,7 @@ Fatal errors propagate to `main` and exit. Per-connector / per-message errors ar All families labeled by `connector_key` + `connector_type` (histogram adds `stage`): -- **Counters**: `iggy_connector_messages_{produced,sent,consumed,processed,filtered}_total` and `iggy_connector_errors_total`. These are the *rendered* names; each is registered without the `_total`, which the OpenMetrics encoder appends. +- **Counters**: `iggy_connector_messages_{produced,sent,consumed,processed,filtered,errors}_total`. - `messages_filtered_total` - intentional drops via transform `Ok(None)`. - `errors_total` - unexpected drops (decode/encode/build failure, missing field, ...) + batch-level failures. - **Histograms**: `iggy_connector_stage_duration_seconds{stage}` (snake_case stage labels - `prepare`, `ffi`, `decode`, `iggy_send`, `state_save`, `total`). Buckets `STAGE_BUCKETS_SECONDS`. Always populated regardless of any flag. Scraped at `/metrics` when `[http.metrics] enabled = true`. @@ -221,7 +221,7 @@ All families labeled by `connector_key` + `connector_type` (histogram adds `stag When adding a metric: -- Add family to `Metrics` struct + `init`, register with name + help text. Never end a `Counter` family's registered name in `_total`: the encoder appends it and the series renders `_total_total`. Gauges get no suffix, so a gauge name may end in `_total` literally. +- Add family to `Metrics` struct + `init`, register with name + help text. - New label sets define `EncodeLabelSet` struct + label enum (hand-impl `EncodeLabelValue` for snake_case values - the derive emits PascalCase). - Histograms: pass `fn() -> Histogram` to `Family::new_with_constructor`. - Add unit tests under `mod tests` with `given_*_when_*_should_*` BDD names. diff --git a/.claude/skills/connector-sink/SKILL.md b/.claude/skills/connector-sink/SKILL.md index 80533036b6..9d40b08323 100644 --- a/.claude/skills/connector-sink/SKILL.md +++ b/.claude/skills/connector-sink/SKILL.md @@ -34,7 +34,8 @@ for getting them to the external system reliably and efficiently. ## Quick reference -- Skeleton: [TEMPLATE.md](TEMPLATE.md) (load on demand when authoring). +- Skeleton: [TEMPLATE.md](TEMPLATE.md) (fill-in-the-blank kit — implement only `TODO(ConnectorDeveloper)`). +- PR pre-flight: [connector-pr-review](../connector-pr-review/SKILL.md). - Exemplars: `stdout_sink` (minimal), `postgres_sink` (DB + transient detection), `http_sink` (validation, batch modes, retry middleware), `mongodb_sink` (atomic counters), `elasticsearch_sink` / `iceberg_sink` (backend-specific idioms). ## Hard rules diff --git a/.claude/skills/connector-sink/TEMPLATE.md b/.claude/skills/connector-sink/TEMPLATE.md index 32a9f97270..19a6956309 100644 --- a/.claude/skills/connector-sink/TEMPLATE.md +++ b/.claude/skills/connector-sink/TEMPLATE.md @@ -1,100 +1,194 @@ -# Sink plugin skeleton +# Sink plugin fill-in-the-blank kit -Boilerplate for a new `core/connectors/sinks/_sink/`. Adapt the -`MySink` / `Client` types to the backend driver you're integrating. +Copy this kit into `core/connectors/sinks/_sink/`. The scaffolding +covers config, secrets, retry, error classification, batching, logging, +and unit-test shape. **You only implement the marked `TODO(ConnectorDeveloper)` +sections:** build a client from the connection string, and push one +batch. + +Also read [SKILL.md](SKILL.md) and pre-flight with +[connector-pr-review](../connector-pr-review/SKILL.md) before `/ready`. + +## Files to create + +```text +core/connectors/sinks/_sink/ +├── Cargo.toml +├── README.md +├── config.toml +└── src/lib.rs +``` + +Add a workspace member, a row in `sinks/README.md`, and a sample under +`runtime/example_config/connectors/`. + +--- ## Cargo.toml ```toml -# Apache 2.0 header (copy verbatim from any existing sink Cargo.toml) +# 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__sink" -version = "0.4.1-edge.1" # match the version most other sinks use +version = "0.4.1-edge.1" edition = "2024" license = "Apache-2.0" publish = false -# ...keywords, description, repository, homepage all identical to existing sinks +description = "Apache Iggy sink connector" +repository = "https://github.com/apache/iggy" +homepage = "https://iggy.apache.org" [package.metadata.cargo-machete] -ignored = ["dashmap", "once_cell"] # used by sink_connector! macro +ignored = ["dashmap", "once_cell"] [lib] -crate-type = ["cdylib", "lib"] # cdylib = runtime-loadable; lib = unit tests +crate-type = ["cdylib", "lib"] [dependencies] -async-trait = { workspace = true } -dashmap = { workspace = true } +async-trait = { workspace = true } +dashmap = { workspace = true } +humantime = { workspace = true } +iggy_common = { workspace = true } iggy_connector_sdk = { workspace = true } -once_cell = { workspace = true } -serde = { workspace = true } -tokio = { workspace = true } -tracing = { workspace = true } -# + your client crate (reqwest, sqlx, mongodb, ...) +once_cell = { workspace = true } +secrecy = { workspace = true } +serde = { workspace = true } +tokio = { workspace = true } +tracing = { workspace = true } +# TODO(ConnectorDeveloper): add your client crate as a workspace dependency +``` + +Run `cargo sort --no-format --workspace` after edits. Keep `Cargo.lock` +churn limited to this crate's deps. + +--- + +## config.toml (example) + +```toml +# Plugin path matches sibling sinks (relative to connectors runtime cwd). +path = "../../target/release/libiggy_connector__sink" + +[[sinks]] +key = "" +enabled = true +# path is also set via IGGY_CONNECTORS_SINK__PATH in integration tests + +[sinks..plugin_config] +# Never commit real secrets. Use env overrides in tests/ops. +connection_string = "scheme://user:pass@host:port/db" +batch_size = 100 +max_retries = 3 +retry_delay = "500ms" +verbose_logging = false ``` -Run `cargo sort --no-format --workspace` after edits. +--- ## src/lib.rs -Code reads top to bottom. Public types first, then `impl`, then private -helpers. +Replace `` / `` and implement only the `TODO(ConnectorDeveloper)` blocks. ```rust -/* Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements... */ // full Apache header +// 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. use async_trait::async_trait; +use humantime::Duration as HumanDuration; use iggy_connector_sdk::{ ConsumedMessage, Error, MessagesMetadata, Sink, TopicMetadata, sink_connector, }; +use secrecy::{ExposeSecret, SecretString}; use serde::{Deserialize, Serialize}; use std::str::FromStr; use std::time::Duration; use tokio::sync::Mutex; use tracing::{debug, error, info, warn}; -sink_connector!(MySink); // generates FFI symbols + version export +sink_connector!(NameSink); -const CONNECTOR_NAME: &str = "My sink"; +const CONNECTOR_NAME: &str = "Name sink"; +const DEFAULT_BATCH_SIZE: u32 = 100; +const DEFAULT_MAX_RETRIES: u32 = 3; // total attempts +const DEFAULT_RETRY_DELAY: &str = "500ms"; + +/// Backend client. Replace with the real driver type. +struct BackendClient { + // TODO(ConnectorDeveloper): fields +} #[derive(Debug, Serialize, Deserialize)] -pub struct MySinkConfig { - pub endpoint: String, +#[serde(deny_unknown_fields)] +pub struct NameSinkConfig { + #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] + pub connection_string: SecretString, pub batch_size: Option, pub max_retries: Option, - pub retry_delay: Option, // humantime, e.g. "500ms" - pub verbose_logging: Option, // mirror runtime's `verbose` flag - // Every runtime-tunable field is Option; defaults applied in new() + pub retry_delay: Option, + pub verbose_logging: Option, + // TODO(ConnectorDeveloper): optional non-secret knobs (table, index, batch_mode, ...) } #[derive(Debug)] -pub struct MySink { +pub struct NameSink { id: u32, - config: MySinkConfig, + config: NameSinkConfig, batch_size: usize, max_retries: u32, retry_delay: Duration, verbose: bool, - client: Option, + client: Option, state: Mutex, } -#[derive(Debug)] +#[derive(Debug, Default)] struct State { messages_processed: u64, errors: u64, } -impl MySink { - pub fn new(id: u32, config: MySinkConfig) -> Self { - let batch_size = config.batch_size.unwrap_or(100) as usize; - let max_retries = config.max_retries.unwrap_or(3); +impl NameSink { + pub fn new(id: u32, config: NameSinkConfig) -> Self { + let batch_size = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE) as usize; + let max_retries = config.max_retries.unwrap_or(DEFAULT_MAX_RETRIES); let retry_delay = config .retry_delay .as_deref() - .and_then(|raw| humantime::Duration::from_str(raw).ok().map(|d| *d)) + .and_then(|raw| HumanDuration::from_str(raw).ok().map(|d| *d)) .unwrap_or_else(|| { - warn!("Invalid retry_delay for {CONNECTOR_NAME} ID: {id}, defaulting to 500ms"); + warn!( + "Invalid retry_delay for {CONNECTOR_NAME} ID: {id}, defaulting to {DEFAULT_RETRY_DELAY}" + ); Duration::from_millis(500) }); let verbose = config.verbose_logging.unwrap_or(false); @@ -106,24 +200,49 @@ impl MySink { retry_delay, verbose, client: None, - state: Mutex::new(State { messages_processed: 0, errors: 0 }), + state: Mutex::new(State::default()), + } + } + + async fn send_batch_with_retry( + &self, + client: &BackendClient, + topic_metadata: &TopicMetadata, + batch: &[ConsumedMessage], + ) -> Result<(), Error> { + let mut attempt = 0u32; + loop { + attempt += 1; + match push_batch(client, topic_metadata, batch).await { + Ok(()) => return Ok(()), + Err(error) if is_permanent(&error) => return Err(error), + Err(error) if attempt >= self.max_retries => return Err(error), + Err(error) => { + warn!( + "{CONNECTOR_NAME} ID: {} retry {attempt}/{}: {error}", + self.id, self.max_retries + ); + tokio::time::sleep(self.retry_delay.saturating_mul(attempt)).await; + } + } } } } #[async_trait] -impl Sink for MySink { +impl Sink for NameSink { async fn open(&mut self) -> Result<(), Error> { + // Structural validation belongs here / in new(), not in consume(). let client = build_client(&self.config) .await .map_err(|e| Error::InitError(format!("client build failed: {e}")))?; - client.ping() + ping(&client) .await .map_err(|e| Error::InitError(format!("connectivity check failed: {e}")))?; self.client = Some(client); info!( - "Opened {CONNECTOR_NAME} connector ID: {}, endpoint: {}", - self.id, self.config.endpoint + "Opened {CONNECTOR_NAME} connector ID: {}, endpoint: ", + self.id ); Ok(()) } @@ -140,28 +259,44 @@ impl Sink for MySink { if self.verbose { info!( - "{CONNECTOR_NAME} ID: {} consuming {} messages from stream: {}, topic: {}, offset: {}, current_offset: {}", - self.id, messages.len(), topic_metadata.stream, topic_metadata.topic, - messages_metadata.partition_id, messages_metadata.current_offset + "{CONNECTOR_NAME} ID: {} consuming {} messages from stream: {}, topic: {}, partition_id: {}, current_offset: {}", + self.id, + messages.len(), + topic_metadata.stream, + topic_metadata.topic, + messages_metadata.partition_id, + messages_metadata.current_offset ); } else { debug!( "{CONNECTOR_NAME} ID: {} consuming {} messages", - self.id, messages.len() + self.id, + messages.len() ); } + // Never swallow a failed batch as Ok(()) — offsets may still advance. let mut last_err: Option = None; for batch in messages.chunks(self.batch_size) { - match self.send_batch(client, batch).await { - Ok(()) => { /* counter */ } + match self + .send_batch_with_retry(client, topic_metadata, batch) + .await + { + Ok(()) => { + let mut state = self.state.lock().await; + state.messages_processed += batch.len() as u64; + } Err(Error::PermanentHttpError(message)) => { error!( "{CONNECTOR_NAME} ID: {} dropping batch (permanent): {message}", self.id ); + let mut state = self.state.lock().await; + state.errors += 1; } Err(error) => { + let mut state = self.state.lock().await; + state.errors += 1; last_err = Some(error); } } @@ -173,9 +308,8 @@ impl Sink for MySink { } async fn close(&mut self) -> Result<(), Error> { - // sqlx pools have `.close().await`; reqwest/mongodb/elasticsearch just drop. if let Some(client) = self.client.take() { - let _ = client; // or `client.close().await;` for sqlx + close_client(client).await; } let state = self.state.lock().await; info!( @@ -186,5 +320,98 @@ impl Sink for MySink { } } -async fn build_client(config: &MySinkConfig) -> Result { /* ... */ } +// ─── Backend surface: implement these ─────────────────────────────────────── + +/// TODO(ConnectorDeveloper): parse `config.connection_string.expose_secret()` and build the client. +async fn build_client(config: &NameSinkConfig) -> Result { + let _secret = config.connection_string.expose_secret(); + Err("TODO(ConnectorDeveloper): build_client".into()) +} + +/// TODO(ConnectorDeveloper): cheap connectivity probe used from open(). +async fn ping(_client: &BackendClient) -> Result<(), String> { + Ok(()) +} + +/// TODO(ConnectorDeveloper): push one batch. Prefer a stable dedup key from +/// `stream:topic:partition:message_id` (or backend natural key). +/// Use `message.payload.try_to_bytes()` — do not clone Payload::Json. +async fn push_batch( + _client: &BackendClient, + _topic_metadata: &TopicMetadata, + _batch: &[ConsumedMessage], +) -> Result<(), Error> { + Err(Error::InitError("TODO(ConnectorDeveloper): push_batch".into())) +} + +/// Map driver errors. Never classify via `err.to_string()` substrings. +fn is_permanent(error: &Error) -> bool { + matches!( + error, + Error::PermanentHttpError(_) | Error::SchemaMismatch(_) | Error::InvalidRecordValue(_) + ) +} + +async fn close_client(_client: BackendClient) { + // sqlx: pool.close().await; most HTTP clients: drop +} + +#[cfg(test)] +mod tests { + use super::*; + + fn test_config() -> NameSinkConfig { + NameSinkConfig { + connection_string: SecretString::from("scheme://localhost/db"), + batch_size: Some(10), + max_retries: Some(2), + retry_delay: Some("10ms".into()), + verbose_logging: Some(false), + } + } + + #[test] + fn given_defaults_should_apply_consts() { + let sink = NameSink::new( + 1, + NameSinkConfig { + connection_string: SecretString::from("scheme://localhost/db"), + batch_size: None, + max_retries: None, + retry_delay: None, + verbose_logging: None, + }, + ); + assert_eq!(sink.batch_size, DEFAULT_BATCH_SIZE as usize); + assert_eq!(sink.max_retries, DEFAULT_MAX_RETRIES); + } + + #[test] + fn given_invalid_retry_delay_should_fall_back_to_default() { + let mut config = test_config(); + config.retry_delay = Some("not-a-duration".into()); + let sink = NameSink::new(1, config); + assert_eq!(sink.retry_delay, Duration::from_millis(500)); + } +} ``` + +--- + +## README.md (required paragraphs) + +Your README must include a **Delivery semantics** section answering: + +1. Transient failure behavior (retry N times, then `Err`) +2. Permanent failure behavior (drop/skip vs fail) +3. Duplication window (usually at-least-once) +4. Dedup key (or "none — duplicates possible") + +Diff README defaults against the `DEFAULT_*` consts before opening the PR. + +--- + +## Before `/ready` + +Run the pre-flight checklist in +[connector-pr-review](../connector-pr-review/SKILL.md#pre-flight-author-checklist). diff --git a/.claude/skills/connector-source/SKILL.md b/.claude/skills/connector-source/SKILL.md index 5627b1812d..36037b1062 100644 --- a/.claude/skills/connector-source/SKILL.md +++ b/.claude/skills/connector-source/SKILL.md @@ -9,8 +9,13 @@ A **source** is a Rust `cdylib` that implements `iggy_connector_sdk::Source` and exposes FFI symbols via the `source_connector!` macro. The runtime calls `poll()` in a loop, applies transforms, encodes via the configured `Schema`, sends to -Apache Iggy, and persists the returned `ConnectorState` after every -successful send. +Apache Iggy, and persists the state `poll()` returned - but only after +the send succeeds. Only one batch is ever in flight: the runtime does +not call `poll()` again until it has reported `Ack` or `Nack` for the +current one via `on_batch_result()` (source batch acknowledgment, see PR #3855 +for the full contract). See +[State persistence](#state-persistence-stage-in-poll-commit-in-on_batch_result) +below. > **Universal connector rules** (SecretString, benchmark, verbose flag, drop accounting, filter contract, exemplar patterns) live in > [connectors-overview](../connectors-overview/SKILL.md). This skill @@ -27,14 +32,15 @@ successful send. ## STOP and ask the user before -- Changing the SDK trait surface (`Source::open` / `poll` / `close`) - that's an SDK change. +- Changing the SDK trait surface (`Source::open` / `poll` / `on_batch_result` / `close`) - that's an SDK change, and `poll`/`on_batch_result` are also an FFI change (`iggy_source_handle_v2`, `iggy_source_batch_result` - breaks every pre-built plugin `.so`). - Adding a long-running side task in the plugin - the runtime owns lifecycle. orphans survive `close()`. - Persisting unbounded state - `State` is rewritten every batch. - Adding a source that requires authoritative offsets external to Apache Iggy without coordinating retention. ## Quick reference -- Skeleton: [TEMPLATE.md](TEMPLATE.md) (load on demand). +- Skeleton: [TEMPLATE.md](TEMPLATE.md) (fill-in-the-blank kit — implement only `TODO(ConnectorDeveloper)`). +- PR pre-flight: [connector-pr-review](../connector-pr-review/SKILL.md). - Exemplars: `random_source` (minimal + canonical state tests), `postgres_source` (cursor / delete-after-read / processed-column modes, restart-survives-state tests), `elasticsearch_source` (scroll cursor), `influxdb_source` (time-series scan). ## Hard rules @@ -57,12 +63,61 @@ let persisted = { // brief write }; ``` -### State persistence +### State persistence: stage in `poll()`, commit in `on_batch_result()` + +Source connectors use a one-in-flight-batch ACK/NACK contract (#3855) +between the plugin and the runtime: + +1. `poll()` returns messages and *candidate* state without committing + cursor changes or destructive operations (deletes, mark-processed). +2. The runtime sends the batch to Apache Iggy and waits for the + producer result. +3. After a successful send, the runtime persists the candidate state + to `{state_path}/source_{key}.state`. +4. The runtime calls `on_batch_result(SourceBatchResult::Ack)`. A send + or state-save failure calls `on_batch_result(SourceBatchResult::Nack)` + instead. +5. `on_batch_result()` commits or discards the plugin's staged work + before the next `poll()` starts. The SDK allows only one batch in + flight - it will not call `poll()` again until `on_batch_result()` + for the current batch has returned. + +Canonical pattern (`sources/random_source/src/lib.rs`): + +```rust +pending_state: Mutex>, // staged, not yet committed + +async fn poll(&self) -> Result { + // ... fetch ... + let candidate_state = State { cursor: next_cursor }; + *self.pending_state.lock().await = Some(candidate_state.clone()); + Ok(ProducedMessages { + schema: Schema::Json, + messages, + state: Some(ConnectorState::serialize(&candidate_state, NAME, self.id)?), + }) +} + +async fn on_batch_result(&self, result: SourceBatchResult) -> Result<(), Error> { + let candidate_state = self.pending_state.lock().await.take(); + if result == SourceBatchResult::Ack + && let Some(candidate_state) = candidate_state + { + *self.state.lock().await = candidate_state; + } + // Nack: drop candidate_state, committed self.state is untouched - + // the same range is polled again. + Ok(()) +} +``` - `ConnectorState` is `Vec` via MessagePack (`rmp_serde`). Use `ConnectorState::serialize(&state, NAME, id)` + `ConnectorState::deserialize::(NAME, id)`. Both return `Option` and log on failure (non-fatal). -- Runtime saves to `{state_path}/source_{key}.state` only after a successful Iggy send. Between `poll()` returning and the runtime persisting the save, a crash leaves the same cursor for the next poll - downstream must tolerate at-least-once. -- **Always return state in every `ProducedMessages`**, including empty polls. Empty results still need to advance watermarks (timestamp sources) or affirm "nothing new." +- **The default `on_batch_result` is a no-op.** Only override it - and only then does staging via `pending_state` matter - if `poll()` advances a cursor or performs destructive work (delete-after-read, mark-processed). A source with no staged work (e.g. a pure generator) can rely on the default. +- Returning `Err` from `on_batch_result` **stops the SDK from polling further** - a failed rollback must not be allowed to silently advance to the next batch. +- **Always return state in every `ProducedMessages`**, including empty polls that made progress. Return `state: None` for an empty poll that made *no* progress - this avoids an unnecessary state write and cannot persist state left over from a failed batch. - Keep `State` small - rewritten every batch. No unbounded vecs. +- NACK handling must discard staged cursor changes and staged delete/mark operations so polling redelivers the batch. The SDK retries NACKed batches with capped exponential backoff and stops the source after repeated consecutive NACKs. +- Crash recovery is at-least-once at every point except after the plugin has processed the ACK (see the SDK README's crash-point table, `core/connectors/sdk/README.md#source-delivery-acknowledgment`, for the full breakdown). ### Sleep first @@ -86,18 +141,20 @@ Match `ProducedMessages.schema` to the bytes in `messages[i].payload`: ### Concurrency - Runtime spawns ONE `poll()` task per source. No concurrent `poll()`. +- Only one batch is ever in flight: the SDK does not call `poll()` again until `on_batch_result()` has returned for the previous batch (up to a 30s result timeout, after which the SDK treats it as a Nack). - Don't spawn your own long-running Tokio tasks - runtime owns lifecycle. ### Errors -| Scenario | Variant | -| ------------------------------------------- | ------------------------------------------------- | -| Bad config in `new()`/`open()` | `Error::InitError` | -| Cannot reach external system at startup | `Error::InitError` or `Error::Connection` | -| Transient fetch failure (retry-worthy) | `Error::Connection` or `Error::HttpRequestFailed` | -| Permanent fetch failure (auth, schema gone) | `Error::PermanentHttpError` | -| Row failed to serialize | `Error::Serialization(...)` | -| State serialization failed | log + skip (non-fatal) | +| Scenario | Variant | +| --------------------------------------------------- | ------------------------------------------------- | +| Bad config in `new()`/`open()` | `Error::InitError` | +| Cannot reach external system at startup | `Error::InitError` or `Error::Connection` | +| Transient fetch failure (retry-worthy) | `Error::Connection` or `Error::HttpRequestFailed` | +| Permanent fetch failure (auth, schema gone) | `Error::PermanentHttpError` | +| Row failed to serialize | `Error::Serialization(...)` | +| State serialization failed | log + skip (non-fatal) | +| `on_batch_result()` failed to roll back staged work | `Err` - stops the SDK from polling further | Returning `Err` from `poll()` is only logged by the SDK's FFI bridge (`sdk/src/source.rs::handle_messages`) - the loop continues, the next @@ -127,15 +184,20 @@ Iggy consumer-loop labels use literal API names (`offset=`, `current_offset=`). 1. `async fn poll(&mut self)` - won't compile. Use `&self` + `Mutex`. 2. Holding `state.lock()` across the fetch I/O - blocks `close()`, causes shutdown timeouts. 3. Forgetting to sleep - 100% CPU on idle source. -4. Returning state only on success - state should advance on empty polls too. -5. Unbounded data in `State` - rewritten every batch. keep O(constant). -6. `std::sync::Mutex` - blocks the executor. Use `tokio::sync::Mutex`. -7. Not setting `ProducedMessage.id` when a stable ID exists - loses idempotency. -8. Spawning side tasks - the runtime owns the scheduler. +4. Returning `state: None` for an empty poll that *did* make progress (e.g. advanced a watermark) - only a no-progress empty poll should return `None`. +5. Committing a cursor or destructive work (delete/mark-processed) directly in `poll()` instead of staging it and applying it in `on_batch_result()` on `Ack` - a Nack (send or state-save failure) has nothing to discard, and the batch is redelivered against already-mutated state. +6. Unbounded data in `State` - rewritten every batch. keep O(constant). +7. `std::sync::Mutex` - blocks the executor. Use `tokio::sync::Mutex`. +8. Not setting `ProducedMessage.id` when a stable ID exists - loses idempotency. +9. Spawning side tasks - the runtime owns the scheduler. ## Tests -Mandatory four canonical source state tests (see [connector-testing](../connector-testing/SKILL.md) for the full pattern). Copy from `sources/random_source/src/lib.rs::tests`. Plus config defaults, payload building, schema selection. +Mandatory six canonical source tests (see [connector-testing](../connector-testing/SKILL.md) for the full pattern): the four +state tests (restore / no-state / invalid-state / round-trip) plus `given_ack_when_batch_is_staged_should_commit_candidate_state` +and `given_nack_when_batch_is_staged_should_keep_committed_state`. Copy from `sources/random_source/src/lib.rs::tests`. Plus +config defaults, payload building, schema selection. A source relying on the default no-op `on_batch_result` (no staged work) +may skip the ack/nack pair. Integration tests under `core/integration/tests/connectors//` for any source backed by external infra. Use `#[iggy_harness]` + a `TestFixture` backed by `testcontainers-modules`. Reference: `core/integration/tests/connectors/postgres/postgres_source.rs` (multi-mode tests) + `restart.rs` (state survives restart). diff --git a/.claude/skills/connector-source/TEMPLATE.md b/.claude/skills/connector-source/TEMPLATE.md index 7a27953654..c5db2183b7 100644 --- a/.claude/skills/connector-source/TEMPLATE.md +++ b/.claude/skills/connector-source/TEMPLATE.md @@ -1,25 +1,95 @@ -# Source plugin skeleton +# Source plugin fill-in-the-blank kit -Boilerplate for a new `core/connectors/sources/_source/`. Adapt -the `MySource` / `Client` / row types to the backend driver. +Copy this kit into `core/connectors/sources/_source/`. The +scaffolding covers config, secrets, sleep-first poll, lock discipline, +staged/committed state ser·de (ACK/NACK batch acknowledgment, #3855), +retry classification, logging, and the six canonical state tests. +**You only implement the marked `TODO(ConnectorDeveloper)` sections:** build a +client from the connection string, and fetch the next batch (advancing +a cursor). -`Cargo.toml` is identical to a sink's (see -[connector-sink/TEMPLATE.md](../connector-sink/TEMPLATE.md)) - only -the crate name suffix changes (`iggy_connector__source`) and the -upstream client dep. +Also read [SKILL.md](SKILL.md) and pre-flight with +[connector-pr-review](../connector-pr-review/SKILL.md) before `/ready`. + +## Files to create + +```text +core/connectors/sources/_source/ +├── Cargo.toml +├── README.md +├── config.toml +└── src/lib.rs +``` + +Add a workspace member, a row in `sources/README.md`, and a sample under +`runtime/example_config/connectors/`. + +--- + +## Cargo.toml + +Same shape as the sink kit (`cdylib` + `lib`, workspace deps, Apache +header). Only the package name suffix changes: + +```toml +name = "iggy_connector__source" +# ... identical metadata / machete ignored / crate-type ... +# TODO(ConnectorDeveloper): add your client crate as a workspace dependency +``` + +--- + +## config.toml (example) + +```toml +path = "../../target/release/libiggy_connector__source" + +[[sources]] +key = "" +enabled = true + +[sources..plugin_config] +connection_string = "scheme://user:pass@host:port/db" +poll_interval = "5s" +batch_size = 100 +max_retries = 3 +retry_delay = "500ms" +verbose_logging = false +``` + +Defaults in this file must match `DEFAULT_*` consts in code. + +--- ## src/lib.rs -Code reads top to bottom. Public types first, `impl Source` after, -helpers below. +Replace `` / `` and implement only the `TODO(ConnectorDeveloper)` blocks. ```rust -/* Apache 2.0 header */ +// 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. use async_trait::async_trait; +use humantime::Duration as HumanDuration; use iggy_connector_sdk::{ - ConnectorState, Error, ProducedMessage, ProducedMessages, Schema, Source, source_connector, + ConnectorState, Error, ProducedMessage, ProducedMessages, Schema, Source, + source::SourceBatchResult, source_connector, }; +use secrecy::{ExposeSecret, SecretString}; use serde::{Deserialize, Serialize}; use std::str::FromStr; use std::time::Duration; @@ -27,135 +97,224 @@ use tokio::sync::Mutex; use tokio::time::sleep; use tracing::{debug, error, info, warn}; -source_connector!(MySource); +source_connector!(NameSource); -const CONNECTOR_NAME: &str = "My source"; +const CONNECTOR_NAME: &str = "Name source"; +const DEFAULT_POLL_INTERVAL: &str = "5s"; +const DEFAULT_BATCH_SIZE: u32 = 100; +const DEFAULT_MAX_RETRIES: u32 = 3; // total attempts +const DEFAULT_RETRY_DELAY: &str = "500ms"; + +struct BackendClient { + // TODO(ConnectorDeveloper): fields +} #[derive(Debug, Serialize, Deserialize)] -pub struct MySourceConfig { - pub endpoint: String, - pub poll_interval: Option, // humantime, "10s" default +#[serde(deny_unknown_fields)] +pub struct NameSourceConfig { + #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] + pub connection_string: SecretString, + pub poll_interval: Option, pub batch_size: Option, pub max_retries: Option, pub retry_delay: Option, pub verbose_logging: Option, + // TODO(ConnectorDeveloper): optional non-secret knobs (query, table, index, ...) } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] struct State { - cursor: Option, // WAL LSN, scroll id, timestamp, ... - last_offset: u64, + /// Opaque backend cursor (LSN, scroll id, timestamp, PK, ...). Keep O(1). + cursor: Option, messages_produced: u64, } #[derive(Debug)] -pub struct MySource { +pub struct NameSource { id: u32, - config: MySourceConfig, + config: NameSourceConfig, poll_interval: Duration, + batch_size: usize, + max_retries: u32, + retry_delay: Duration, verbose: bool, - client: Option, + client: Option, state: Mutex, + /// Staged in `poll()`, committed to `state` on `Ack`, discarded on `Nack` + /// (source batch acknowledgment, #3855). Only one batch is ever staged + /// at a time - the SDK enforces one in-flight batch. + pending_state: Mutex>, +} + +struct FetchedBatch { + messages: Vec, + /// Next cursor, staged via `pending_state` and committed only after + /// `on_batch_result(SourceBatchResult::Ack)`. + next_cursor: Option, } -impl MySource { - pub fn new(id: u32, config: MySourceConfig, state: Option) -> Self { - let raw_interval = config.poll_interval.clone().unwrap_or_else(|| "10s".into()); - let poll_interval = humantime::Duration::from_str(&raw_interval) +impl NameSource { + pub fn new(id: u32, config: NameSourceConfig, state: Option) -> Self { + let raw_interval = config + .poll_interval + .clone() + .unwrap_or_else(|| DEFAULT_POLL_INTERVAL.into()); + let poll_interval = HumanDuration::from_str(&raw_interval) .map(|d| *d) .unwrap_or_else(|_| { - warn!("Invalid poll_interval for {CONNECTOR_NAME} ID: {id}, defaulting to 10s"); - Duration::from_secs(10) + warn!( + "Invalid poll_interval for {CONNECTOR_NAME} ID: {id}, defaulting to {DEFAULT_POLL_INTERVAL}" + ); + Duration::from_secs(5) + }); + let batch_size = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE) as usize; + let max_retries = config.max_retries.unwrap_or(DEFAULT_MAX_RETRIES); + let retry_delay = config + .retry_delay + .as_deref() + .and_then(|raw| HumanDuration::from_str(raw).ok().map(|d| *d)) + .unwrap_or_else(|| { + warn!( + "Invalid retry_delay for {CONNECTOR_NAME} ID: {id}, defaulting to {DEFAULT_RETRY_DELAY}" + ); + Duration::from_millis(500) }); - let verbose = config.verbose_logging.unwrap_or(false); let restored = state .and_then(|s| s.deserialize::(CONNECTOR_NAME, id)) - .inspect(|s| info!( - "Restored state for {CONNECTOR_NAME} ID: {id}, last_offset: {}, cursor: {:?}", - s.last_offset, s.cursor - )); + .inspect(|s| { + info!( + "Restored state for {CONNECTOR_NAME} ID: {id}, cursor: {:?}, messages_produced: {}", + s.cursor, s.messages_produced + ); + }); Self { id, config, poll_interval, + batch_size, + max_retries, + retry_delay, verbose, client: None, state: Mutex::new(restored.unwrap_or(State { cursor: None, - last_offset: 0, messages_produced: 0, })), + pending_state: Mutex::new(None), + } + } + + async fn fetch_with_retry( + &self, + client: &BackendClient, + cursor: Option, + ) -> Result { + let mut attempt = 0u32; + loop { + attempt += 1; + match fetch_batch(client, cursor.as_deref(), self.batch_size).await { + Ok(batch) => return Ok(batch), + Err(error) if is_permanent(&error) => return Err(error), + Err(error) if attempt >= self.max_retries => return Err(error), + Err(error) => { + warn!( + "{CONNECTOR_NAME} ID: {} retry {attempt}/{}: {error}", + self.id, self.max_retries + ); + sleep(self.retry_delay.saturating_mul(attempt)).await; + } + } } } } #[async_trait] -impl Source for MySource { +impl Source for NameSource { async fn open(&mut self) -> Result<(), Error> { + // Validate query/cursor shape here — not on first poll after sleep. let client = build_client(&self.config) .await .map_err(|e| Error::InitError(format!("client build failed: {e}")))?; + ping(&client) + .await + .map_err(|e| Error::InitError(format!("connectivity check failed: {e}")))?; self.client = Some(client); info!( - "Opened {CONNECTOR_NAME} connector ID: {}, endpoint: {}", - self.id, self.config.endpoint + "Opened {CONNECTOR_NAME} connector ID: {}, endpoint: ", + self.id ); Ok(()) } async fn poll(&self) -> Result { - sleep(self.poll_interval).await; // sleep first - backpressure - - let cursor = { self.state.lock().await.cursor.clone() }; // brief read - - let fetched = self.fetch_since(cursor.as_deref()).await?; // no lock held - - let mut messages = Vec::with_capacity(fetched.len()); - let mut next_cursor = None; - for row in fetched { - let payload = simd_json::to_vec(&row).map_err(|e| - Error::Serialization(format!("row serialize: {e}")) - )?; - messages.push(ProducedMessage { - id: Some(row.id as u128), - checksum: None, - timestamp: None, - origin_timestamp: Some(row.created_at_ns), - headers: None, - payload, - }); - next_cursor = Some(row.cursor_value); - } + // Sleep FIRST or an idle source spins the CPU. + sleep(self.poll_interval).await; + + let Some(client) = self.client.as_ref() else { + return Err(Error::InitError("client not initialized".into())); + }; + + // Brief lock read → drop → I/O. Never hold across upstream await. + let cursor = { self.state.lock().await.cursor.clone() }; + + let fetched = self.fetch_with_retry(client, cursor).await?; if self.verbose { info!( - "{CONNECTOR_NAME} ID: {} produced {} messages, next cursor: {:?}", - self.id, messages.len(), next_cursor + "{CONNECTOR_NAME} ID: {} polled {} messages, next_cursor: {:?}", + self.id, + fetched.messages.len(), + fetched.next_cursor + ); + } else { + debug!( + "{CONNECTOR_NAME} ID: {} polled {} messages", + self.id, + fetched.messages.len() ); } - let persisted = { // brief write - let mut state = self.state.lock().await; - state.messages_produced += messages.len() as u64; - if let Some(c) = next_cursor { - state.cursor = Some(c); - } - ConnectorState::serialize(&*state, CONNECTOR_NAME, self.id) + // Stage the candidate state - do NOT commit it to `self.state` here, and do + // NOT delete/mark upstream rows here either. The runtime sends the batch and + // saves this staged state; `on_batch_result()` below commits it (Ack) or + // discards it (Nack) before the next poll() runs. Committing directly in + // poll() would leave nothing to roll back on a Nack. + let messages_produced = self.state.lock().await.messages_produced; + let candidate_state = State { + cursor: fetched.next_cursor, + messages_produced: messages_produced + fetched.messages.len() as u64, }; + let state_bytes = ConnectorState::serialize(&candidate_state, CONNECTOR_NAME, self.id); + *self.pending_state.lock().await = Some(candidate_state); Ok(ProducedMessages { - schema: Schema::Json, - messages, - state: persisted, + schema: Schema::Json, // TODO(ConnectorDeveloper): match actual payload bytes + messages: fetched.messages, + state: state_bytes, }) } + // Commits or discards the batch staged in poll() above. The default + // trait impl is a no-op - override is mandatory whenever poll() stages + // a cursor or destructive work, which this template always does. + async fn on_batch_result(&self, result: SourceBatchResult) -> Result<(), Error> { + let candidate_state = self.pending_state.lock().await.take(); + if result == SourceBatchResult::Ack + && let Some(candidate_state) = candidate_state + { + *self.state.lock().await = candidate_state; + } + // Nack: candidate_state is dropped, self.state (committed) is untouched, + // so the next poll() re-fetches from the same committed cursor. + Ok(()) + } + async fn close(&mut self) -> Result<(), Error> { if let Some(client) = self.client.take() { - let _ = client; // or `client.close().await;` for sqlx pools + close_client(client).await; } let state = self.state.lock().await; info!( @@ -166,5 +325,187 @@ impl Source for MySource { } } -async fn build_client(config: &MySourceConfig) -> Result { /* ... */ } +// ─── Backend surface: implement these ─────────────────────────────────────── + +/// TODO(ConnectorDeveloper): parse `config.connection_string.expose_secret()` and build the client. +async fn build_client(config: &NameSourceConfig) -> Result { + let _secret = config.connection_string.expose_secret(); + Err("TODO(ConnectorDeveloper): build_client".into()) +} + +/// TODO(ConnectorDeveloper): cheap connectivity probe used from open(). +async fn ping(_client: &BackendClient) -> Result<(), String> { + Ok(()) +} + +/// TODO(ConnectorDeveloper): fetch up to `limit` rows after `cursor`. +/// Set `ProducedMessage.id` from a stable natural key (never random UUID). +/// Set `origin_timestamp` when the backend has event time (nanoseconds). +async fn fetch_batch( + _client: &BackendClient, + _cursor: Option<&str>, + _limit: usize, +) -> Result { + Err(Error::InitError("TODO(ConnectorDeveloper): fetch_batch".into())) +} + +fn is_permanent(error: &Error) -> bool { + matches!( + error, + Error::PermanentHttpError(_) | Error::SchemaMismatch(_) | Error::InvalidConfigValue(_) + ) +} + +async fn close_client(_client: BackendClient) {} + +#[cfg(test)] +mod tests { + use super::*; + + fn test_config() -> NameSourceConfig { + NameSourceConfig { + connection_string: SecretString::from("scheme://localhost/db"), + poll_interval: Some("100ms".into()), + batch_size: Some(10), + max_retries: Some(2), + retry_delay: Some("10ms".into()), + verbose_logging: Some(false), + } + } + + #[test] + fn given_persisted_state_should_restore_cursor() { + let state = State { + cursor: Some("cursor-1".into()), + messages_produced: 7, + }; + let bytes = rmp_serde::to_vec(&state).expect("serialize"); + let source = NameSource::new(1, test_config(), Some(ConnectorState(bytes))); + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let restored = source.state.lock().await; + assert_eq!(restored.cursor.as_deref(), Some("cursor-1")); + assert_eq!(restored.messages_produced, 7); + }); + } + + #[test] + fn given_no_state_should_start_fresh() { + let source = NameSource::new(1, test_config(), None); + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let restored = source.state.lock().await; + assert!(restored.cursor.is_none()); + assert_eq!(restored.messages_produced, 0); + }); + } + + #[test] + fn given_invalid_state_should_start_fresh() { + let invalid = ConnectorState(b"not valid msgpack".to_vec()); + let source = NameSource::new(1, test_config(), Some(invalid)); + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let restored = source.state.lock().await; + assert!(restored.cursor.is_none()); + assert_eq!(restored.messages_produced, 0); + }); + } + + #[test] + fn state_should_be_serializable_and_deserializable() { + let original = State { + cursor: Some("c".into()), + messages_produced: 3, + }; + let bytes = rmp_serde::to_vec(&original).unwrap(); + let restored: State = rmp_serde::from_slice(&bytes).unwrap(); + assert_eq!(original, restored); + } + + #[test] + fn given_defaults_should_apply_consts() { + let source = NameSource::new( + 1, + NameSourceConfig { + connection_string: SecretString::from("scheme://localhost/db"), + poll_interval: None, + batch_size: None, + max_retries: None, + retry_delay: None, + verbose_logging: None, + }, + None, + ); + assert_eq!(source.batch_size, DEFAULT_BATCH_SIZE as usize); + assert_eq!(source.max_retries, DEFAULT_MAX_RETRIES); + assert_eq!(source.poll_interval, Duration::from_secs(5)); + } + + // Stages `pending_state` directly rather than going through poll() - poll()'s + // TODO(ConnectorDeveloper) fetch is unimplemented in this template, but on_batch_result's + // commit/discard contract is independently testable and must stay covered once + // the backend is filled in. + + #[test] + fn given_ack_when_batch_is_staged_should_commit_candidate_state() { + let source = NameSource::new(1, test_config(), None); + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let candidate = State { + cursor: Some("cursor-2".into()), + messages_produced: 5, + }; + *source.pending_state.lock().await = Some(candidate.clone()); + + source + .on_batch_result(SourceBatchResult::Ack) + .await + .expect("ack should be applied"); + + assert_eq!(*source.state.lock().await, candidate); + assert!(source.pending_state.lock().await.is_none()); + }); + } + + #[test] + fn given_nack_when_batch_is_staged_should_keep_committed_state() { + let source = NameSource::new(1, test_config(), None); + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let committed_before = source.state.lock().await.clone(); + *source.pending_state.lock().await = Some(State { + cursor: Some("cursor-2".into()), + messages_produced: 5, + }); + + source + .on_batch_result(SourceBatchResult::Nack) + .await + .expect("nack should be applied"); + + assert_eq!(*source.state.lock().await, committed_before); + assert!(source.pending_state.lock().await.is_none()); + }); + } +} ``` + +--- + +## README.md (required paragraphs) + +Include a **Delivery semantics** section: + +1. Transient fetch failure → retry N times, then `Err` (loop continues; see `connector-source`) +2. Cursor commits only on `on_batch_result(Ack)` - a Nack (send or state-save failure) discards the staged cursor and redelivers the batch +3. Whether destructive work (delete/mark-processed) is staged in `poll()` and applied only in `on_batch_result()` on Ack (standard - no loss window to document), or must happen earlier for some architectural reason (if so, document the loss window explicitly) +4. Dedup key for `ProducedMessage.id` (or "none") + +--- + +## Before `/ready` + +Run the pre-flight checklist in +[connector-pr-review](../connector-pr-review/SKILL.md#pre-flight-author-checklist). +Mandatory: the six canonical state tests above must stay green. diff --git a/.claude/skills/connectors-overview/SKILL.md b/.claude/skills/connectors-overview/SKILL.md index 230ad5354b..d119dcf409 100644 --- a/.claude/skills/connectors-overview/SKILL.md +++ b/.claude/skills/connectors-overview/SKILL.md @@ -25,7 +25,7 @@ Repo-wide rules (Apache headers, fmt/sort/clippy order, idiomatic Rust traits, i ## STOP and ask the user before -- Bumping `iggy_connector_sdk` MAJOR version or changing any FFI signature in `sdk/src/{sink,source}.rs` - breaks every pre-built plugin `.so`. +- Bumping `iggy_connector_sdk` MAJOR version or changing any FFI signature in `sdk/src/{sink,source}.rs` - breaks every pre-built plugin `.so`. Source FFI is `iggy_source_handle_v2` (batch ID carried to the runtime callback) + `iggy_source_batch_result` (plugin-exported ACK/NACK, #3855). - Changing the runtime's wire conventions (postcard FFI payload structs, default consumer group naming, plugin path resolution). - Modifying `runtime/src/state.rs` save protocol (atomic rename + fsync ordering) - corruption risk. - Renaming or repurposing a `Schema` variant - decoders/encoders pinned to wire bytes. @@ -42,27 +42,31 @@ Both compile as `cdylib` shared libraries (`.so`/`.dylib`/`.dll`) loaded by the ```text ┌─ optional transforms ─┐ External ──poll──▶ SOURCE ──FFI──▶ RUNTIME ──encode──▶ Apache Iggy stream - system plugin ▲ ▲ - │ │ - state save (msgpack) │ - │ - ┌─ optional transforms ─┐ │ + system plugin ▲ ▲ + │ │ + Ack/Nack (on_batch_result) │ + │ state save (msgpack) + └────────────────────┘ + ┌─ optional transforms ─┐ Apache Iggy stream ──decode──▶ RUNTIME ──FFI──▶ SINK ──write──▶ External plugin system ``` +Source is a request/response FFI, not fire-and-forget: after the runtime sends the batch and saves the state `poll()` staged, it reports `SourceBatchResult::Ack` or `::Nack` back to the plugin via `iggy_source_batch_result` (#3855) before calling `poll()` again. See [connector-source](../connector-source/SKILL.md#state-persistence-stage-in-poll-commit-in-on_batch_result) for the full handshake. + Headers set on the source side ride through transforms (which may modify, drop, or pass them) and arrive at the sink with `BTreeMap` preserved deterministically. ## Which skill to load -| Task | Skill | -| --------------------------------------------------- | --------------------- | -| Write a new sink plugin | `connector-sink` | -| Write a new source plugin | `connector-source` | -| Add schema / decoder / encoder / SDK trait surface | `connector-sdk` | -| Change runtime internals (FFI, manager, state, ...) | `connector-runtime` | -| Add a transform (field-level or format conversion) | `connector-transform` | -| Write unit / integration tests for any of the above | `connector-testing` | +| Task | Skill | +| ---------------------------------------------------- | --------------------- | +| Write a new sink plugin | `connector-sink` | +| Write a new source plugin | `connector-source` | +| Add schema / decoder / encoder / SDK trait surface | `connector-sdk` | +| Change runtime internals (FFI, manager, state, ...) | `connector-runtime` | +| Add a transform (field-level or format conversion) | `connector-transform` | +| Write unit / integration tests for any of the above | `connector-testing` | +| Review a sink/source PR / pre-flight before `/ready` | `connector-pr-review` | ## Stick to conventions @@ -96,24 +100,14 @@ The connectors codebase is intentionally repetitive across plugins. Cross-plugin ### Secrets -Any credential-bearing field (connection strings, API keys, bearer tokens, AWS keys) must be `SecretString` from the `secrecy` crate. Plain `String` for a credential is a review-blocker: `SecretString` redacts on `Debug`, so it is what keeps a credential out of a log line that formats the whole config. - -**`serde_secret::serialize_secret` EXPOSES the secret. It does not redact.** It calls `expose_secret()` and writes the plaintext. `SecretString` deliberately has no `Serialize` impl, and that absence is the protection - so adding `serialize_with` is what *unblocks* the derive and turns a compile-time guarantee into plaintext output. Use it only where the plaintext is the point: a wire payload, a persisted config, an API response that exposes credentials by design. - -So the default for a plugin config struct is **derive `Deserialize`, but not `Serialize`**. `Deserialize` is required: the SDK glue deserializes the config into the plugin's own struct (`sdk/src/{sink,source}.rs` call `serde_json::from_str::` under a `DeserializeOwned` bound). - -What never happens is the return trip. The runtime holds plugin configuration as a `serde_json::Value` - parsed from TOML, posted as JSON to the control API, or injected by env var - and hands that across the FFI, so nothing re-serializes the plugin's struct. Leaving `Serialize` off makes that compiler-enforced instead of convention-enforced (`sources/http_source/src/lib.rs::HttpSourceConfig` does this, and comments the omission so nobody adds it back). - -Pattern: +Any credential-bearing field (connection strings, API keys, bearer tokens, AWS keys) must be `SecretString` from the `secrecy` crate, with the workspace serde wrapper applied so `Debug` and serialization both redact. Runtime exposes plugin configs over the `/stats` HTTP surface via serialization - plain `String` leaks the secret to anyone who can hit the endpoint. Plain `String` for a credential is a review-blocker. Pattern (from `sinks/postgres_sink/src/lib.rs::PostgresSinkConfig`): ```rust use secrecy::{ExposeSecret, SecretString}; -// `Deserialize` only. Nothing re-serializes a plugin config, and leaving -// `Serialize` off is what makes the credential unserializable rather than -// merely un-serialized. -#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct MyConfig { + #[serde(serialize_with = "iggy_common::serde_secret::serialize_secret")] pub connection_string: SecretString, } @@ -123,13 +117,7 @@ let pool = PgPoolOptions::new() .await?; ``` -If a config struct genuinely needs `Serialize`, `serde_secret::serialize_redacted` (and `serialize_optional_redacted`) write `[REDACTED]` in place of the value. Reach for `serialize_secret` only when the caller must get the real thing back. The sinks and sources listed below predate that helper and use the exposing one; the annotation is inert today, but it is not the protection it looks like. - -Note that none of this protects the credential from the runtime's own control API, which returns plugin configuration verbatim - see #3802. Plugin-side annotations are inert there because the runtime never routes through them. - -Plugin-side uses of the exposing helpers: `sinks/{postgres,mongodb,elasticsearch,influxdb,s3,surrealdb}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. - -That list is plugin-side only, not an inventory of every caller in the tree, and the others are not all mistakes: `runtime/src/api/config.rs` puts `serialize_secret` on `HttpConfig::api_key` (inert for the same reason), and several `core/common` wire-payload types (login, create-user, change-password, PAT) use these helpers by design, because there the credential *is* the payload. +In-tree uses: `sinks/{postgres,mongodb,elasticsearch,influxdb,delta}_sink`, `sources/{postgres,elasticsearch,influxdb}_source`. ### Errors @@ -181,7 +169,7 @@ JSON log format, parser unit tests). | Real-infra sink + integration tests | `sinks/postgres_sink/` + `integration/tests/connectors/postgres/postgres_sink.rs` | | Feature-rich sink config (validation, batch modes, retry) | `sinks/http_sink/` | | Atomic counters on hot path | `sinks/mongodb_sink/` | -| Simplest source (4 canonical state tests) | `sources/random_source/` | +| Simplest source (6 canonical state + ACK/NACK tests) | `sources/random_source/` | | Real-infra source | `sources/postgres_source/` + `integration/tests/connectors/postgres/postgres_source.rs` | Read the relevant exemplar end-to-end before writing or modifying a connector. @@ -209,7 +197,7 @@ Each implemented in at least one in-tree plugin or runtime path. | `flume::unbounded()` channel | `runtime/src/source.rs::spawn_source_handler` / `source_forwarding_loop` | MPSC handoff from SDK async task to runtime loop | | `tokio::sync::watch::channel(())` | `sdk/src/{sink,source}.rs`, `runtime/src/sink.rs`, `runtime/src/manager/*` | One-shot shutdown broadcast | | `dashmap::DashMap` | `runtime/src/manager/sink.rs`, `source.rs::SOURCE_SENDERS`, SDK `INSTANCES` | Lock-free concurrent keyed access | -| `secrecy::SecretString` + `iggy_common::serde_secret::serialize_secret` | `sinks/postgres_sink::PostgresSinkConfig::connection_string` | `Debug` redacts; `serialize_secret` EXPOSES | +| `secrecy::SecretString` + `iggy_common::serde_secret::serialize_secret` | `sinks/postgres_sink::PostgresSinkConfig::connection_string` | Auto-redact on Debug/Display + serialization | ## Drop accounting @@ -222,13 +210,21 @@ Each implemented in at least one in-tree plugin or runtime path. - `&mut self` on `Sink::consume` or `Source::poll` impls - won't compile, flag any creative workaround. - `std::sync::Mutex` held across `.await` - swap for `tokio::sync::Mutex`. - Missing `[lib] crate-type = ["cdylib", "lib"]` in plugin `Cargo.toml`. -- Source plugin without the four canonical state tests (see `connector-testing`). +- Source plugin without the four canonical state tests, or without ACK/NACK tests when `on_batch_result` is overridden (see `connector-testing`). +- Source `poll()` committing a cursor or destructive work directly instead of staging it for `on_batch_result` to commit on `Ack` / discard on `Nack`. - New silent message drop without a metric increment. - Wrapping `format!()` around args passed to `error!`/`warn!`/`info!`/`debug!` - eager `format!` allocates even when level filters the line out. Pass args directly: `error!("foo: {x}")` or `error!(error = %x, "foo")`. - Logging a connection string, API key, or token. - Plain `String` for a credential field - use `SecretString`. - `tokio::spawn` inside plugin code - runtime owns lifecycle. - `std::time::SystemTime::now()` in transforms - non-deterministic, breaks tests. +- Returning `Ok(())` from `consume` after a failed batch (offsets can still advance). +- Random UUIDs as message IDs / dedup keys. +- Classifying retryability via `err.to_string()` substring matches. +- README defaults that disagree with code consts. +- Invented config knob names (`request_timeout`, `retry_max_delay`) instead of the canon in `connector-pr-review`. + +For the full PR review checklist (blockers, delivery-semantics paragraph, pre-flight paste), load [connector-pr-review](../connector-pr-review/SKILL.md). ## File map diff --git a/core/connectors/BLOG_POST.md b/core/connectors/BLOG_POST.md index fe8b45ea1c..02db07fa96 100644 --- a/core/connectors/BLOG_POST.md +++ b/core/connectors/BLOG_POST.md @@ -72,7 +72,7 @@ passing `cargo test`: round-trip — plus the two ACK/NACK tests, plus config validation and the circuit-breaker short-circuit path). -What's left is marked `TODO(Developer)` in each crate's `src/lib.rs`: +What's left is marked `TODO(ConnectorDeveloper)` in each crate's `src/lib.rs`: one spot for a sink (`push_batch()`), two for a source (`build_raw_client()` if you're not talking HTTP, and `fetch_records()`). Everything else — the parts that used to eat a @@ -81,7 +81,7 @@ review round — is already done. ## Using one Copy the crate, rename the package and the directory, add it to the -workspace `members` list, fill in the `TODO(Developer)` spots, and +workspace `members` list, fill in the `TODO(ConnectorDeveloper)` spots, and update `config.toml` for your system. Each crate's own `README.md` walks through the exact steps. Both templates already build, `clippy --all-targets -- -D warnings` clean, and pass their tests as committed diff --git a/core/connectors/sinks/README.md b/core/connectors/sinks/README.md index 990cbe59f8..c0d786f332 100644 --- a/core/connectors/sinks/README.md +++ b/core/connectors/sinks/README.md @@ -16,7 +16,7 @@ Sink connectors are responsible for writing data from Iggy streams to external s | **postgres_sink** | Stores messages in PostgreSQL database tables with configurable schemas | | **quickwit_sink** | Indexes messages in Quickwit search engine for log analytics | | **s3_sink** | Writes messages to Amazon S3 and S3-compatible stores (MinIO, R2, B2, DO Spaces) | -| **sink_template** | Fill-in-the-blank starting point for a new sink; framework/security plumbing done, one `TODO(Developer)` spot left | +| **sink_template** | Fill-in-the-blank starting point for a new sink; framework/security plumbing done, one `TODO(ConnectorDeveloper)` spot left | | **stdout_sink** | Prints messages to standard output (useful for debugging and development) | | **surrealdb_sink** | Writes messages into SurrealDB with deterministic record IDs for idempotent replay | diff --git a/core/connectors/sinks/sink_template/README.md b/core/connectors/sinks/sink_template/README.md index 16547f5d14..002409da6b 100644 --- a/core/connectors/sinks/sink_template/README.md +++ b/core/connectors/sinks/sink_template/README.md @@ -32,7 +32,7 @@ blog post for the checklist this template is built against. ## What you need to fill in -Search for `TODO(Developer)` in `src/lib.rs` — there is exactly one spot: +Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there is exactly one spot: **`push_batch()`** — build the request/write that actually sends one chunk of messages to your destination, using `self.config.connection_string` (and @@ -55,7 +55,7 @@ in `open()` in favor of whatever connectivity check your driver offers. 1. Copy this directory, rename it and the package in `Cargo.toml` (`iggy_connector__sink`), and add it to the `members` list in the workspace root `Cargo.toml`. -2. Fill in the `TODO(Developer)` section(s). +2. Fill in the `TODO(ConnectorDeveloper)` section(s). 3. Update `config.toml` with your real `connection_string` and `target`, and any settings specific to your system; delete `auth_token` if you don't need it, or add fields of your own the same way (see diff --git a/core/connectors/sinks/sink_template/src/lib.rs b/core/connectors/sinks/sink_template/src/lib.rs index 20a28aa9d6..152121c5c7 100644 --- a/core/connectors/sinks/sink_template/src/lib.rs +++ b/core/connectors/sinks/sink_template/src/lib.rs @@ -25,7 +25,7 @@ //! are chunked by a configurable batch size instead of shipped as one //! unbounded request. //! -//! There is exactly **one** place you need to touch, marked `TODO(Developer)`: +//! There is exactly **one** place you need to touch, marked `TODO(ConnectorDeveloper)`: //! `TemplateSink::push_batch()` — build the request/write that actually //! pushes one chunk of messages to your destination, using //! `self.config.connection_string` (and `self.config.target`, if your @@ -83,7 +83,7 @@ const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct TemplateSinkConfig { - /// TODO(Developer): document the exact shape this connector expects, e.g. + /// TODO(ConnectorDeveloper): document the exact shape this connector expects, e.g. /// "https://api.example.com" or "postgres://user:pass@host:5432/db". /// `SecretString` because DSNs commonly embed credentials — never plain /// `String` for this field, see `PostgresSinkConfig::connection_string` @@ -188,7 +188,7 @@ impl TemplateSink { } } - /// TODO(Developer): build your actual client/connection here using + /// TODO(ConnectorDeveloper): build your actual client/connection here using /// `self.config.connection_string` (and `self.config.auth_token`, if /// your destination needs it). This template builds a plain /// `reqwest::Client` to hand to `build_retry_client` — if you're not @@ -205,7 +205,7 @@ impl TemplateSink { .map_err(|e| Error::Connection(format!("failed to build HTTP client: {e}"))) } - /// TODO(Developer): push one chunk of already-batched messages to your + /// TODO(ConnectorDeveloper): push one chunk of already-batched messages to your /// destination using `self.config.connection_string` and /// `self.config.target`, via `self.client` (already retry-wrapped). /// Distinguish permanent failures (bad schema, destination rejects the @@ -221,7 +221,7 @@ impl TemplateSink { ) -> Result<(), Error> { let _ = (client, batch); // remove once implemented Err(Error::InitError( - "TemplateSink::push_batch is not implemented yet — see the TODO(Developer) comment in \ + "TemplateSink::push_batch is not implemented yet — see the TODO(ConnectorDeveloper) comment in \ template_sink/src/lib.rs" .to_string(), )) diff --git a/core/connectors/sources/README.md b/core/connectors/sources/README.md index 640cd0ad5c..f79b253513 100644 --- a/core/connectors/sources/README.md +++ b/core/connectors/sources/README.md @@ -12,7 +12,7 @@ Source connectors are responsible for ingesting data from external sources into | **influxdb_source** | Polls InfluxDB with cursor-based timestamp tracking; supports V2 (Flux, annotated CSV) and V3 (SQL, JSONL) | | **postgres_source** | Reads rows from PostgreSQL tables with multiple strategies: delete after read, mark as processed, or timestamp tracking | | **random_source** | Generates random test messages (useful for testing and development) | -| **source_template** | Fill-in-the-blank starting point for a new source; framework/security plumbing done, two `TODO(Developer)` spots left | +| **source_template** | Fill-in-the-blank starting point for a new source; framework/security plumbing done, two `TODO(ConnectorDeveloper)` spots left | The source is represented by the single `Source` trait, which defines the basic interface for all source connectors. It provides methods for initializing the source, reading data from it, and closing the source. diff --git a/core/connectors/sources/source_template/README.md b/core/connectors/sources/source_template/README.md index f4a39d3bac..415e2c210a 100644 --- a/core/connectors/sources/source_template/README.md +++ b/core/connectors/sources/source_template/README.md @@ -29,7 +29,7 @@ blog post for the checklist this template is built against. ## What you need to fill in -Search for `TODO(Developer)` in `src/lib.rs` — there are exactly two spots: +Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there are exactly two spots: 1. **`build_raw_client()`** — if your source isn't HTTP, replace the `reqwest::Client` construction with your driver's connection/pool setup @@ -50,7 +50,7 @@ Search for `TODO(Developer)` in `src/lib.rs` — there are exactly two spots: 1. Copy this directory, rename it and the package in `Cargo.toml` (`iggy_connector__source`), and add it to the `members` list in the workspace root `Cargo.toml`. -2. Fill in the two `TODO(Developer)` sections. +2. Fill in the two `TODO(ConnectorDeveloper)` sections. 3. Update `config.toml` with your real `connection_string` and any settings specific to your system; delete `auth_token` if you don't need it, or add fields of your own the same way (see `TemplateSourceConfig`). diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 24f52b44c1..57a2c576b0 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -25,7 +25,7 @@ //! a dropped/nacked batch can be re-polled instead of silently lost. //! //! There are exactly **two** places you need to touch, each marked -//! `TODO(Developer)`: +//! `TODO(ConnectorDeveloper)`: //! 1. `TemplateSource::connect()` — build your actual client/connection //! from `config.connection_string` (and `config.auth_token`, if used). //! 2. `TemplateSource::fetch_records()` — fetch up to `batch_size` new @@ -84,7 +84,7 @@ const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct TemplateSourceConfig { - /// TODO(Developer): document the exact shape this connector expects, e.g. + /// TODO(ConnectorDeveloper): document the exact shape this connector expects, e.g. /// "https://api.example.com" or "postgres://user:pass@host:5432/db". /// `SecretString` because DSNs commonly embed credentials — never plain /// `String` for this field, see `PostgresSinkConfig::connection_string` @@ -201,7 +201,7 @@ impl TemplateSource { ConnectorState::serialize(state, CONNECTOR_NAME, self.id) } - /// TODO(Developer): build your actual client/connection here using + /// TODO(ConnectorDeveloper): build your actual client/connection here using /// `self.config.connection_string` (and `self.config.auth_token`, if /// your system needs it). This template builds a plain `reqwest::Client` /// to hand to `build_retry_client` — if you're not talking HTTP, replace @@ -217,7 +217,7 @@ impl TemplateSource { .map_err(|e| Error::Connection(format!("failed to build HTTP client: {e}"))) } - /// TODO(Developer): fetch up to `self.batch_size` new records from your + /// TODO(ConnectorDeveloper): fetch up to `self.batch_size` new records from your /// external system, ordered after `cursor` (`None` means "from the /// beginning" or "from now" — whichever is right for your source). /// Use `self.config.connection_string` as the base address and @@ -233,7 +233,7 @@ impl TemplateSource { ) -> Result, Error> { let _ = (client, cursor); // remove once implemented Err(Error::InitError( - "TemplateSource::fetch_records is not implemented yet — see the TODO(Developer) comment \ + "TemplateSource::fetch_records is not implemented yet — see the TODO(ConnectorDeveloper) comment \ in template_source/src/lib.rs" .to_string(), )) From 4cf2ba22d803a47563607938060ed60b63d6b3d9 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Tue, 25 Aug 2026 13:10:36 -0400 Subject: [PATCH 03/10] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- core/connectors/sinks/sink_template/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/connectors/sinks/sink_template/src/lib.rs b/core/connectors/sinks/sink_template/src/lib.rs index 152121c5c7..7b62d6cddb 100644 --- a/core/connectors/sinks/sink_template/src/lib.rs +++ b/core/connectors/sinks/sink_template/src/lib.rs @@ -118,7 +118,7 @@ pub struct TemplateSinkConfig { pub timeout: Option, pub max_retries: Option, pub retry_delay: Option, - pub retry_max_delay: Option, + pub max_retry_delay: Option, pub max_open_retries: Option, pub open_retry_max_delay: Option, pub circuit_breaker_threshold: Option, From ff0365a0853e2f6413bdc03e096897f686339703 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Tue, 25 Aug 2026 22:56:03 +0530 Subject: [PATCH 04/10] Changed the license headers --- .../connectors/sinks/sink_template/Cargo.toml | 40 +++++++++---------- .../sources/source_template/Cargo.toml | 40 +++++++++---------- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/core/connectors/sinks/sink_template/Cargo.toml b/core/connectors/sinks/sink_template/Cargo.toml index 8ab161bd1e..b4ab09edc3 100644 --- a/core/connectors/sinks/sink_template/Cargo.toml +++ b/core/connectors/sinks/sink_template/Cargo.toml @@ -1,23 +1,23 @@ -# 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. -# -# TEMPLATE — rename the package (and this directory) to -# `iggy_connector__sink` before publishing, and update the -# `[[sinks]]` entry you add to the workspace root Cargo.toml accordingly. +// 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. +// +//! TEMPLATE — rename the package (and this directory) to +//! `iggy_connector__sink` before publishing, and update the +//! `[[sinks]]` entry you add to the workspace root Cargo.toml accordingly. [package] name = "iggy_connector_template_sink" diff --git a/core/connectors/sources/source_template/Cargo.toml b/core/connectors/sources/source_template/Cargo.toml index a54d7cbccc..392504dacf 100644 --- a/core/connectors/sources/source_template/Cargo.toml +++ b/core/connectors/sources/source_template/Cargo.toml @@ -1,23 +1,23 @@ -# 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. -# -# TEMPLATE — rename the package (and this directory) to -# `iggy_connector__source` before publishing, and update the -# `[[sources]]` entry you add to the workspace root Cargo.toml accordingly. +// 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. +// +//! TEMPLATE — rename the package (and this directory) to +//! `iggy_connector__source` before publishing, and update the +//! `[[sources]]` entry you add to the workspace root Cargo.toml accordingly. [package] name = "iggy_connector_template_source" From 5fe30bc6009556304e0196cde7a56c1631813f84 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 10:42:21 +0530 Subject: [PATCH 05/10] Fixing the review comments --- .claude/skills/connector-pr-review/SKILL.md | 2 +- .claude/skills/connector-sink/TEMPLATE.md | 6 + .claude/skills/connector-source/TEMPLATE.md | 6 + .claude/skills/connectors-overview/SKILL.md | 2 +- .../connectors/sink_template.toml | 1 + .../connectors/source_template.toml | 1 + .../connectors/sinks/sink_template/Cargo.toml | 40 ++-- core/connectors/sinks/sink_template/README.md | 30 ++- .../sinks/sink_template/config.toml | 1 + .../connectors/sinks/sink_template/src/lib.rs | 197 ++++++++++++++---- .../sources/source_template/Cargo.toml | 40 ++-- .../sources/source_template/README.md | 26 ++- .../sources/source_template/config.toml | 1 + .../sources/source_template/src/lib.rs | 76 ++++++- 14 files changed, 309 insertions(+), 120 deletions(-) diff --git a/.claude/skills/connector-pr-review/SKILL.md b/.claude/skills/connector-pr-review/SKILL.md index c386099c79..01f2c38915 100644 --- a/.claude/skills/connector-pr-review/SKILL.md +++ b/.claude/skills/connector-pr-review/SKILL.md @@ -153,7 +153,7 @@ These are "cheap" but burn full review rounds when missed. | Request timeout | `timeout` | Not `request_timeout` | | Retry attempts | `max_retries` | Total attempts, default 3 | | Base backoff | `retry_delay` | humantime `Option` | -| Backoff ceiling | `max_retry_delay` | Not `retry_max_delay` | +| Backoff ceiling | `retry_max_delay` | Matches SDK's `ConnectivityConfig::open_retry_max_delay` | | Poll cadence (sources) | `poll_interval` | humantime; sleep first | | Plugin verbosity | `verbose_logging` | Mirror runtime `verbose` | | Credentials | `connection_string` / `api_key` / … | Always `SecretString` | diff --git a/.claude/skills/connector-sink/TEMPLATE.md b/.claude/skills/connector-sink/TEMPLATE.md index 19a6956309..cb4bee4d5c 100644 --- a/.claude/skills/connector-sink/TEMPLATE.md +++ b/.claude/skills/connector-sink/TEMPLATE.md @@ -9,6 +9,12 @@ batch. Also read [SKILL.md](SKILL.md) and pre-flight with [connector-pr-review](../connector-pr-review/SKILL.md) before `/ready`. +Prefer starting from a compiling crate over copying this prose kit: +`core/connectors/sinks/sink_template/` implements the same shape as real, +tested code you can `cargo build`/`cargo test` immediately, with the same +`TODO(ConnectorDeveloper)` markers. Use this kit instead only when copying +a whole crate is more scaffolding than you need. + ## Files to create ```text diff --git a/.claude/skills/connector-source/TEMPLATE.md b/.claude/skills/connector-source/TEMPLATE.md index c5db2183b7..306cba25f2 100644 --- a/.claude/skills/connector-source/TEMPLATE.md +++ b/.claude/skills/connector-source/TEMPLATE.md @@ -11,6 +11,12 @@ a cursor). Also read [SKILL.md](SKILL.md) and pre-flight with [connector-pr-review](../connector-pr-review/SKILL.md) before `/ready`. +Prefer starting from a compiling crate over copying this prose kit: +`core/connectors/sources/source_template/` implements the same shape as +real, tested code you can `cargo build`/`cargo test` immediately, with the +same `TODO(ConnectorDeveloper)` markers. Use this kit instead only when +copying a whole crate is more scaffolding than you need. + ## Files to create ```text diff --git a/.claude/skills/connectors-overview/SKILL.md b/.claude/skills/connectors-overview/SKILL.md index d119dcf409..4f6f5e169e 100644 --- a/.claude/skills/connectors-overview/SKILL.md +++ b/.claude/skills/connectors-overview/SKILL.md @@ -222,7 +222,7 @@ Each implemented in at least one in-tree plugin or runtime path. - Random UUIDs as message IDs / dedup keys. - Classifying retryability via `err.to_string()` substring matches. - README defaults that disagree with code consts. -- Invented config knob names (`request_timeout`, `retry_max_delay`) instead of the canon in `connector-pr-review`. +- Invented config knob names (e.g. `request_timeout` instead of `timeout`) instead of the canon in `connector-pr-review`. For the full PR review checklist (blockers, delivery-semantics paragraph, pre-flight paste), load [connector-pr-review](../connector-pr-review/SKILL.md). diff --git a/core/connectors/runtime/example_config/connectors/sink_template.toml b/core/connectors/runtime/example_config/connectors/sink_template.toml index 90c3937f95..ee9a7bd3de 100644 --- a/core/connectors/runtime/example_config/connectors/sink_template.toml +++ b/core/connectors/runtime/example_config/connectors/sink_template.toml @@ -45,3 +45,4 @@ max_open_retries = 10 open_retry_max_delay = "60s" circuit_breaker_threshold = 5 circuit_breaker_cool_down = "30s" +verbose_logging = false diff --git a/core/connectors/runtime/example_config/connectors/source_template.toml b/core/connectors/runtime/example_config/connectors/source_template.toml index c53439a0ae..02dbe0eab8 100644 --- a/core/connectors/runtime/example_config/connectors/source_template.toml +++ b/core/connectors/runtime/example_config/connectors/source_template.toml @@ -44,3 +44,4 @@ max_open_retries = 10 open_retry_max_delay = "60s" circuit_breaker_threshold = 5 circuit_breaker_cool_down = "30s" +verbose_logging = false diff --git a/core/connectors/sinks/sink_template/Cargo.toml b/core/connectors/sinks/sink_template/Cargo.toml index b4ab09edc3..6ee035f63b 100644 --- a/core/connectors/sinks/sink_template/Cargo.toml +++ b/core/connectors/sinks/sink_template/Cargo.toml @@ -1,23 +1,23 @@ -// 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. -// -//! TEMPLATE — rename the package (and this directory) to -//! `iggy_connector__sink` before publishing, and update the -//! `[[sinks]]` entry you add to the workspace root Cargo.toml accordingly. +# 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. + +# TEMPLATE — rename the package (and this directory) to +# `iggy_connector__sink` before publishing, and update the +# `[[sinks]]` entry you add to the workspace root Cargo.toml accordingly. [package] name = "iggy_connector_template_sink" diff --git a/core/connectors/sinks/sink_template/README.md b/core/connectors/sinks/sink_template/README.md index 002409da6b..86da056d81 100644 --- a/core/connectors/sinks/sink_template/README.md +++ b/core/connectors/sinks/sink_template/README.md @@ -27,21 +27,29 @@ blog post for the checklist this template is built against. `batch_size` instead of sending everything in one unbounded request. - The `sink_connector!` FFI macro invocation and a `Cargo.toml` with the right `crate-type`, workspace-pinned dependencies, and license header. -- Tests for config/identifier validation and the circuit-breaker short-circuit - path. +- `verbose_logging: Option` upgrading the per-batch log line from + `debug!` to `info!`, mirroring the runtime's own `verbose` flag. +- Tests for config/identifier validation, the circuit-breaker short-circuit + path, the `verbose_logging` flag, and `consume()`'s batch loop end to end. ## What you need to fill in -Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there is exactly one spot: +Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there is exactly one +spot that requires code, plus two more that are conditional/documentation: -**`push_batch()`** — build the request/write that actually sends one chunk -of messages to your destination, using `self.config.connection_string` (and -`self.config.target`, already validated by the time this runs) via -`self.client` (already retry-wrapped). Distinguish permanent failures (bad -schema, a destination that will reject this payload shape no matter how many -times you retry) from transient ones (network error, 5xx, timeout) by -returning `Error::PermanentHttpError` for the former — see the doc comment on -that variant for why the distinction matters to the circuit breaker. +**`push_batch()`** (required) — build the request/write that actually sends +one chunk of messages to your destination, using +`self.config.connection_string` (and `self.config.target`, already validated +by the time this runs) via `self.client` (already retry-wrapped). Distinguish +permanent failures (bad schema, a destination that will reject this payload +shape no matter how many times you retry) from transient ones (network +error, 5xx, timeout) by returning `Error::PermanentHttpError` for the +former — `consume()` drops and counts a permanent failure instead of +propagating it, so a single bad message can't take the whole connector down; +any other error stops `consume()` and is returned as-is. + +**`connection_string`'s doc comment** (documentation only) — describe the +exact shape your connector expects instead of the generic example. If your destination isn't HTTP, also revisit **`build_raw_client()`**: swap the `reqwest::Client` for your driver's connection/pool setup (see diff --git a/core/connectors/sinks/sink_template/config.toml b/core/connectors/sinks/sink_template/config.toml index 8156530c9d..86c91de246 100644 --- a/core/connectors/sinks/sink_template/config.toml +++ b/core/connectors/sinks/sink_template/config.toml @@ -45,4 +45,5 @@ max_open_retries = 10 open_retry_max_delay = "60s" circuit_breaker_threshold = 5 circuit_breaker_cool_down = "30s" +verbose_logging = false # auth_token = "replace-me" # uncomment if your destination needs bearer/API-key auth diff --git a/core/connectors/sinks/sink_template/src/lib.rs b/core/connectors/sinks/sink_template/src/lib.rs index 7b62d6cddb..f1ad735068 100644 --- a/core/connectors/sinks/sink_template/src/lib.rs +++ b/core/connectors/sinks/sink_template/src/lib.rs @@ -25,18 +25,23 @@ //! are chunked by a configurable batch size instead of shipped as one //! unbounded request. //! -//! There is exactly **one** place you need to touch, marked `TODO(ConnectorDeveloper)`: -//! `TemplateSink::push_batch()` — build the request/write that actually -//! pushes one chunk of messages to your destination, using -//! `self.config.connection_string` (and `self.config.target`, if your -//! destination has a table/index/collection-shaped name). +//! There is exactly **one** place you need to write code, marked +//! `TODO(ConnectorDeveloper)`: `TemplateSink::push_batch()` — build the +//! request/write that actually pushes one chunk of messages to your +//! destination, using `self.config.connection_string` (and +//! `self.config.target`, if your destination has a table/index/ +//! collection-shaped name). A second `TODO(ConnectorDeveloper)` on the +//! `connection_string` field's doc comment just asks you to describe its +//! expected shape — not code, but worth personalizing too. `grep` for the +//! marker and you'll find both, plus one more on `build_raw_client()` that +//! only applies if your destination isn't HTTP (see below). //! //! This template assumes an HTTP-ish destination and uses `reqwest` wrapped //! by the SDK's retry middleware, because that's what //! `iggy_connector_sdk::retry` is built for and it covers the common case. //! If your destination talks something else (a database, a queue, object -//! storage), swap the client type in `connect()`/`push_batch()` for your -//! driver of choice and lean on its own retry/pooling behavior — keep the +//! storage), swap the client type in `build_raw_client()`/`push_batch()` for +//! your driver of choice and lean on its own retry/pooling behavior — keep the //! surrounding shape (validation in `open()`, circuit breaker, batching, //! identifier validation) unchanged. See `core/connectors/sinks/postgres_sink` //! or `core/connectors/sinks/s3_sink` in this repo for non-HTTP examples of @@ -58,7 +63,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Duration; use tokio::sync::Mutex; -use tracing::{error, info, warn}; +use tracing::{debug, error, info, warn}; sink_connector!(TemplateSink); @@ -102,7 +107,7 @@ pub struct TemplateSinkConfig { /// `Debug`/log output; delete this field if `connection_string` already /// carries all required auth. Read it with `.expose_secret()` (from the /// `secrecy::ExposeSecret` trait) at the one place you actually need the - /// plaintext — e.g. when building an auth header in `connect()`. + /// plaintext — e.g. when building an auth header in `build_raw_client()`. #[serde( default, serialize_with = "iggy_common::serde_secret::serialize_optional_secret" @@ -118,11 +123,16 @@ pub struct TemplateSinkConfig { pub timeout: Option, pub max_retries: Option, pub retry_delay: Option, - pub max_retry_delay: Option, + pub retry_max_delay: Option, pub max_open_retries: Option, pub open_retry_max_delay: Option, pub circuit_breaker_threshold: Option, pub circuit_breaker_cool_down: Option, + + /// Upgrades the per-batch log line from `debug!` to `info!`. Mirrors the + /// runtime's own `verbose` flag; keep the field name `verbose_logging` + /// (see `postgres_sink::PostgresSinkConfig::verbose_logging`). + pub verbose_logging: Option, } /// Rejects anything that isn't a plain alphanumeric/underscore identifier. @@ -158,6 +168,7 @@ pub struct TemplateSink { circuit_breaker: Arc, batch_size_limit: usize, retry_delay: Duration, + verbose: bool, state: Mutex, records_written_total: AtomicU64, } @@ -175,6 +186,7 @@ impl TemplateSink { ), )); let batch_size_limit = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE).max(1); + let verbose = config.verbose_logging.unwrap_or(false); Self { id, @@ -183,6 +195,7 @@ impl TemplateSink { circuit_breaker, batch_size_limit, retry_delay, + verbose, state: Mutex::new(State::default()), records_written_total: AtomicU64::new(0), } @@ -213,7 +226,11 @@ impl TemplateSink { /// (network error, 5xx, timeout — should retry) by returning /// `Error::PermanentHttpError` for the former; see the doc comment on /// that variant in `iggy_connector_sdk::Error` for why the distinction - /// matters to the circuit breaker. + /// matters. `consume()` drops and counts a `PermanentHttpError` batch but + /// keeps processing the rest; any other error stops `consume()` and is + /// returned, which the runtime treats as fatal for the whole connector — + /// so a permanent classification is what keeps one bad message from + /// taking every future batch down with it. async fn push_batch( &self, client: &ClientWithMiddleware, @@ -321,18 +338,33 @@ impl Sink for TemplateSink { let invocation = state.invocations_count; drop(state); - info!( - "{CONNECTOR_NAME} with ID: {} received: {} messages, schema: {}, stream: {}, topic: {}, \ - partition: {}, offset: {}, invocation: {}", - self.id, - messages.len(), - messages_metadata.schema, - topic_metadata.stream, - topic_metadata.topic, - messages_metadata.partition_id, - messages_metadata.current_offset, - invocation - ); + if self.verbose { + info!( + "{CONNECTOR_NAME} with ID: {} received: {} messages, schema: {}, stream: {}, \ + topic: {}, partition: {}, offset: {}, invocation: {}", + self.id, + messages.len(), + messages_metadata.schema, + topic_metadata.stream, + topic_metadata.topic, + messages_metadata.partition_id, + messages_metadata.current_offset, + invocation + ); + } else { + debug!( + "{CONNECTOR_NAME} with ID: {} received: {} messages, schema: {}, stream: {}, \ + topic: {}, partition: {}, offset: {}, invocation: {}", + self.id, + messages.len(), + messages_metadata.schema, + topic_metadata.stream, + topic_metadata.topic, + messages_metadata.partition_id, + messages_metadata.current_offset, + invocation + ); + } if self.circuit_breaker.is_open().await { warn!( @@ -349,13 +381,32 @@ impl Sink for TemplateSink { Error::Connection("client not initialized -- was open() called?".into()) })?; - let mut first_error: Option = None; + // Track the *last* transient error, not the first — an earlier chunk's + // failure may already be stale by the time later chunks run, and the + // circuit breaker below should react to the most recent signal. A + // `PermanentHttpError` batch (bad schema, will never succeed on retry) + // is dropped and counted here rather than kept as `last_err`: letting + // it propagate would return `Err` from `consume()`, and the runtime + // treats any `Err` here as fatal for the whole connector (see + // `runtime/src/sink.rs::consume_messages`), not just a retry of that + // one batch — so a single unprocessable message would take down every + // future batch instead of just being skipped. + let mut last_err: Option = None; let mut written = 0u64; let mut failed = 0u64; for batch in messages.chunks(self.batch_size_limit) { match self.push_batch(client, batch).await { Ok(()) => written += batch.len() as u64, + Err(Error::PermanentHttpError(message)) => { + failed += batch.len() as u64; + error!( + "{CONNECTOR_NAME} connector with ID: {} dropping a batch of {} \ + (permanent): {message}", + self.id, + batch.len() + ); + } Err(err) => { failed += batch.len() as u64; error!( @@ -363,23 +414,24 @@ impl Sink for TemplateSink { self.id, batch.len() ); - if first_error.is_none() { - first_error = Some(err); - } + last_err = Some(err); } } } // Record the circuit breaker outcome once per `consume()` call, not - // once per chunk — recording success partway through would reset - // the failure counter mid-consume and prevent the breaker from - // opening on a batch with sustained, mixed-success chunks. - match &first_error { - None => self.circuit_breaker.record_success(), - Some(e) if !matches!(e, Error::PermanentHttpError(_)) => { - self.circuit_breaker.record_failure().await; - } - Some(_) => {} + // once per chunk — recording success partway through would reset the + // failure counter mid-consume and prevent the breaker from opening on + // a batch with sustained, mixed-success chunks. A transient failure + // always trips it. A batch where every failure was permanent records + // neither success nor failure — nothing demonstrated the destination + // is reachable, but a schema/data problem isn't a connectivity signal + // either. A batch with at least one real write (or no messages at + // all) counts as success. + match &last_err { + Some(_) => self.circuit_breaker.record_failure().await, + None if written > 0 || messages.is_empty() => self.circuit_breaker.record_success(), + None => {} } let mut state = self.state.lock().await; @@ -389,7 +441,7 @@ impl Sink for TemplateSink { self.records_written_total .fetch_add(written, Ordering::Relaxed); - match first_error { + match last_err { None => Ok(()), Some(err) => Err(err), } @@ -428,9 +480,24 @@ mod tests { open_retry_max_delay: Some("100ms".to_string()), circuit_breaker_threshold: Some(3), circuit_breaker_cool_down: Some("50ms".to_string()), + verbose_logging: None, } } + #[test] + fn given_verbose_logging_enabled_should_set_verbose_flag() { + let mut config = test_config(); + config.verbose_logging = Some(true); + let sink = TemplateSink::new(1, config); + assert!(sink.verbose); + } + + #[test] + fn given_verbose_logging_disabled_should_not_set_verbose_flag() { + let sink = TemplateSink::new(1, test_config()); + assert!(!sink.verbose); + } + #[tokio::test] async fn open_rejects_empty_connection_string() { let mut config = test_config(); @@ -486,19 +553,57 @@ mod tests { sink.circuit_breaker.record_failure().await; assert!(sink.circuit_breaker.is_open().await); - let topic_metadata = TopicMetadata { - stream: "s".to_string(), - topic: "t".to_string(), - }; - let messages_metadata = MessagesMetadata { - partition_id: 1, - current_offset: 0, - schema: iggy_connector_sdk::Schema::Json, - }; + let (topic_metadata, messages_metadata) = topic_and_messages_metadata(); let result = sink .consume(&topic_metadata, messages_metadata, Vec::new()) .await; assert!(matches!(result, Err(Error::CannotStoreData(_)))); } + + fn consumed_message() -> ConsumedMessage { + ConsumedMessage { + id: 1, + offset: 0, + checksum: 0, + timestamp: 0, + origin_timestamp: 0, + headers: None, + payload: iggy_connector_sdk::Payload::Raw(b"payload".to_vec()), + } + } + + fn topic_and_messages_metadata() -> (TopicMetadata, MessagesMetadata) { + ( + TopicMetadata { + stream: "s".to_string(), + topic: "t".to_string(), + }, + MessagesMetadata { + partition_id: 1, + current_offset: 0, + schema: iggy_connector_sdk::Schema::Json, + }, + ) + } + + // push_batch() is an unimplemented TODO(ConnectorDeveloper) stub that + // always returns Error::InitError — not a PermanentHttpError — so a + // non-empty consume() call exercises the "last_err" (retryable) branch + // of the chunking loop end to end, including the failed-count and + // circuit-breaker bookkeeping. + #[tokio::test] + async fn consume_with_unimplemented_push_batch_returns_err_and_counts_failure() { + let mut sink = TemplateSink::new(1, test_config()); + sink.open().await.expect("open should succeed"); + let (topic_metadata, messages_metadata) = topic_and_messages_metadata(); + + let result = sink + .consume(&topic_metadata, messages_metadata, vec![consumed_message()]) + .await; + + assert!(matches!(result, Err(Error::InitError(_)))); + assert_eq!(sink.state.lock().await.messages_failed, 1); + assert_eq!(sink.state.lock().await.messages_written, 0); + } } diff --git a/core/connectors/sources/source_template/Cargo.toml b/core/connectors/sources/source_template/Cargo.toml index 392504dacf..dd29dbacd7 100644 --- a/core/connectors/sources/source_template/Cargo.toml +++ b/core/connectors/sources/source_template/Cargo.toml @@ -1,23 +1,23 @@ -// 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. -// -//! TEMPLATE — rename the package (and this directory) to -//! `iggy_connector__source` before publishing, and update the -//! `[[sources]]` entry you add to the workspace root Cargo.toml accordingly. +# 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. + +# TEMPLATE — rename the package (and this directory) to +# `iggy_connector__source` before publishing, and update the +# `[[sources]]` entry you add to the workspace root Cargo.toml accordingly. [package] name = "iggy_connector_template_source" diff --git a/core/connectors/sources/source_template/README.md b/core/connectors/sources/source_template/README.md index 415e2c210a..0ae77ac2cc 100644 --- a/core/connectors/sources/source_template/README.md +++ b/core/connectors/sources/source_template/README.md @@ -25,32 +25,38 @@ blog post for the checklist this template is built against. on `Nack` so a failed delivery gets re-polled instead of silently lost. - The `source_connector!` FFI macro invocation and a `Cargo.toml` with the right `crate-type`, workspace-pinned dependencies, and license header. -- Tests for config validation and the Ack/Nack state-commit behavior. +- `verbose_logging: Option` upgrading the per-poll log line from + `debug!` to `info!`, mirroring the runtime's own `verbose` flag. +- The six canonical state/Ack-Nack tests, plus config validation and the + `verbose_logging` flag. ## What you need to fill in -Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there are exactly two spots: +Search for `TODO(ConnectorDeveloper)` in `src/lib.rs` — there are exactly +three spots: one required, one conditional, one documentation-only. -1. **`build_raw_client()`** — if your source isn't HTTP, replace the +1. **`fetch_records()`** (required) — fetch up to `self.batch_size` new + records from your system, ordered after `cursor` (`None` = start from the + beginning, or from "now" — whichever fits your source). Map each result + to a `FetchedRecord { cursor_value, payload }`, using something + monotonically increasing as `cursor_value` (a timestamp, an ID, a page + token) — that's what lets the cursor-staging logic advance correctly. +2. **`build_raw_client()`** (only if your source isn't HTTP) — replace the `reqwest::Client` construction with your driver's connection/pool setup (see `core/connectors/sources/postgres_source` for a real non-HTTP example), store it on `TemplateSource` (you'll need to add a field — `client: Option` here is HTTP-specific), and adjust or remove the `check_connectivity_with_retry` call in `open()` in favor of whatever connectivity check your driver offers. -2. **`fetch_records()`** — fetch up to `self.batch_size` new records from - your system, ordered after `cursor` (`None` = start from the beginning, - or from "now" — whichever fits your source). Map each result to a - `FetchedRecord { cursor_value, payload }`, using something monotonically - increasing as `cursor_value` (a timestamp, an ID, a page token) — that's - what lets the cursor-staging logic advance correctly. +3. **`connection_string`'s doc comment** (documentation only) — describe + the exact shape your connector expects instead of the generic example. ## Using it 1. Copy this directory, rename it and the package in `Cargo.toml` (`iggy_connector__source`), and add it to the `members` list in the workspace root `Cargo.toml`. -2. Fill in the two `TODO(ConnectorDeveloper)` sections. +2. Fill in `fetch_records()` (and `build_raw_client()` if not HTTP). 3. Update `config.toml` with your real `connection_string` and any settings specific to your system; delete `auth_token` if you don't need it, or add fields of your own the same way (see `TemplateSourceConfig`). diff --git a/core/connectors/sources/source_template/config.toml b/core/connectors/sources/source_template/config.toml index 53ac14874e..721abb0464 100644 --- a/core/connectors/sources/source_template/config.toml +++ b/core/connectors/sources/source_template/config.toml @@ -44,4 +44,5 @@ max_open_retries = 10 open_retry_max_delay = "60s" circuit_breaker_threshold = 5 circuit_breaker_cool_down = "30s" +verbose_logging = false # auth_token = "replace-me" # uncomment if your source needs bearer/API-key auth diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 57a2c576b0..9191f0fc2d 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -24,18 +24,20 @@ //! staged in `poll()` and only committed in `on_batch_result()` on an ACK so //! a dropped/nacked batch can be re-polled instead of silently lost. //! -//! There are exactly **two** places you need to touch, each marked -//! `TODO(ConnectorDeveloper)`: -//! 1. `TemplateSource::connect()` — build your actual client/connection -//! from `config.connection_string` (and `config.auth_token`, if used). -//! 2. `TemplateSource::fetch_records()` — fetch up to `batch_size` new -//! records from your external system, starting after `cursor`. +//! There is exactly **one** place you need to write code, marked +//! `TODO(ConnectorDeveloper)`: `TemplateSource::fetch_records()` — fetch up +//! to `batch_size` new records from your external system, starting after +//! `cursor`. `TemplateSource::build_raw_client()` carries a second +//! `TODO(ConnectorDeveloper)` too, but only applies if your source isn't +//! HTTP (see below); a third, on the `connection_string` field's doc +//! comment, just asks you to describe its expected shape — not code. +//! `grep` for the marker and you'll find all three. //! //! This template assumes an HTTP-ish source and uses `reqwest` wrapped by //! the SDK's retry middleware, because that's what `iggy_connector_sdk::retry` //! is built for and it covers the common case. If your source talks to //! something else (a database, a queue, a filesystem), swap the client type -//! in `connect()`/`fetch_records()` for your driver of choice and lean on +//! in `build_raw_client()`/`fetch_records()` for your driver of choice and lean on //! its own retry/pooling behavior — keep the surrounding shape (validation //! in `open()`, circuit breaker, cursor staging, batching) unchanged. See //! `core/connectors/sources/postgres_source` in this repo for a real @@ -58,7 +60,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Duration; use tokio::sync::Mutex; -use tracing::{error, info, warn}; +use tracing::{debug, error, info, warn}; source_connector!(TemplateSource); @@ -96,7 +98,7 @@ pub struct TemplateSourceConfig { /// `Debug`/log output; delete this field if `connection_string` already /// carries all required auth. Read it with `.expose_secret()` (from the /// `secrecy::ExposeSecret` trait) at the one place you actually need the - /// plaintext — e.g. when building an auth header in `connect()`. + /// plaintext — e.g. when building an auth header in `build_raw_client()`. #[serde( default, serialize_with = "iggy_common::serde_secret::serialize_optional_secret" @@ -118,6 +120,11 @@ pub struct TemplateSourceConfig { pub open_retry_max_delay: Option, pub circuit_breaker_threshold: Option, pub circuit_breaker_cool_down: Option, + + /// Upgrades the per-poll log line from `debug!` to `info!`. Mirrors the + /// runtime's own `verbose` flag; keep the field name `verbose_logging` + /// (see `postgres_source::PostgresSourceConfig::verbose_logging`). + pub verbose_logging: Option, } // ── Internal state ────────────────────────────────────────────────────────── @@ -149,6 +156,7 @@ pub struct TemplateSource { batch_size: u32, poll_interval: Duration, retry_delay: Duration, + verbose: bool, state: Mutex, pending_state: Mutex>, records_produced: AtomicU64, @@ -173,6 +181,7 @@ impl TemplateSource { ), )); let batch_size = config.batch_size.unwrap_or(DEFAULT_BATCH_SIZE).max(1); + let verbose = config.verbose_logging.unwrap_or(false); let restored_state = state .and_then(|s| s.deserialize::(CONNECTOR_NAME, id)) @@ -191,6 +200,7 @@ impl TemplateSource { batch_size, poll_interval, retry_delay, + verbose, state: Mutex::new(restored_state.unwrap_or_default()), pending_state: Mutex::new(None), records_produced: AtomicU64::new(0), @@ -371,7 +381,6 @@ impl Source for TemplateSource { let mut messages = Vec::with_capacity(records.len()); let mut new_cursor = cursor; for record in records { - new_cursor = Some(record.cursor_value); let Ok(payload) = serde_json::to_vec(&record.payload) else { error!( "Failed to serialize a record fetched by {CONNECTOR_NAME} connector with ID: {}", @@ -379,6 +388,11 @@ impl Source for TemplateSource { ); continue; }; + // Only advance the candidate cursor once the record actually made it + // into `messages` - advancing on a dropped (unserializable) record + // would stage a cursor past data that was never produced, and a + // later Ack would commit past it permanently. + new_cursor = Some(record.cursor_value); messages.push(ProducedMessage { id: None, headers: None, @@ -389,6 +403,17 @@ impl Source for TemplateSource { }); } + if messages.is_empty() { + // Every fetched record failed to serialize - no progress was + // made, so there's nothing to stage and no reason to write + // state, same as the `records.is_empty()` case above. + return Ok(ProducedMessages { + schema: Schema::Json, + messages: Vec::new(), + state: None, + }); + } + let candidate_state = State { cursor: new_cursor }; let persisted_state = self.serialize_state(&candidate_state).ok_or_else(|| { Error::Serialization(format!( @@ -401,6 +426,20 @@ impl Source for TemplateSource { self.records_produced .fetch_add(messages.len() as u64, Ordering::Relaxed); + if self.verbose { + info!( + "{CONNECTOR_NAME} connector with ID: {} produced {} messages", + self.id, + messages.len() + ); + } else { + debug!( + "{CONNECTOR_NAME} connector with ID: {} produced {} messages", + self.id, + messages.len() + ); + } + Ok(ProducedMessages { schema: Schema::Json, messages, @@ -477,9 +516,24 @@ mod tests { open_retry_max_delay: Some("100ms".to_string()), circuit_breaker_threshold: Some(3), circuit_breaker_cool_down: Some("50ms".to_string()), + verbose_logging: None, } } + #[test] + fn given_verbose_logging_enabled_should_set_verbose_flag() { + let mut config = test_config(); + config.verbose_logging = Some(true); + let source = TemplateSource::new(1, config, None); + assert!(source.verbose); + } + + #[test] + fn given_verbose_logging_disabled_should_not_set_verbose_flag() { + let source = TemplateSource::new(1, test_config(), None); + assert!(!source.verbose); + } + #[tokio::test] async fn open_rejects_empty_connection_string() { let mut config = test_config(); @@ -568,7 +622,7 @@ mod tests { assert!(source.pending_state.lock().await.is_none()); } - #[tokio::test] + #[tokio::test(start_paused = true)] async fn poll_returns_empty_without_error_when_circuit_is_open() { let source = TemplateSource::new(1, test_config(), None); source.circuit_breaker.record_failure().await; From 9ab9c50164fa5c8dd3bb9916f95e866567796227 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 12:00:49 +0530 Subject: [PATCH 06/10] Fixing cargo machete --- Cargo.lock | 1 - core/connectors/sinks/sink_template/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fa3a3efb13..af69db4cdd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7238,7 +7238,6 @@ dependencies = [ "reqwest-middleware", "secrecy", "serde", - "serde_json", "tokio", "tracing", ] diff --git a/core/connectors/sinks/sink_template/Cargo.toml b/core/connectors/sinks/sink_template/Cargo.toml index 6ee035f63b..886962dc6d 100644 --- a/core/connectors/sinks/sink_template/Cargo.toml +++ b/core/connectors/sinks/sink_template/Cargo.toml @@ -50,7 +50,6 @@ reqwest = { workspace = true } reqwest-middleware = { workspace = true } secrecy = { workspace = true } serde = { workspace = true } -serde_json = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } From 6986980a26e42a6d4a32b0d52c8ec9a088ce8062 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 12:42:35 +0530 Subject: [PATCH 07/10] making clickable url --- core/connectors/sinks/sink_template/src/lib.rs | 2 +- core/connectors/sources/source_template/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/connectors/sinks/sink_template/src/lib.rs b/core/connectors/sinks/sink_template/src/lib.rs index f1ad735068..47bfa83676 100644 --- a/core/connectors/sinks/sink_template/src/lib.rs +++ b/core/connectors/sinks/sink_template/src/lib.rs @@ -89,7 +89,7 @@ const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; #[serde(deny_unknown_fields)] pub struct TemplateSinkConfig { /// TODO(ConnectorDeveloper): document the exact shape this connector expects, e.g. - /// "https://api.example.com" or "postgres://user:pass@host:5432/db". + /// "" or "postgres://user:pass@host:5432/db". /// `SecretString` because DSNs commonly embed credentials — never plain /// `String` for this field, see `PostgresSinkConfig::connection_string` /// in `sinks/postgres_sink` for the same pattern. diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 9191f0fc2d..978811e3d1 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -87,7 +87,7 @@ const DEFAULT_CIRCUIT_BREAKER_COOL_DOWN: &str = "30s"; #[serde(deny_unknown_fields)] pub struct TemplateSourceConfig { /// TODO(ConnectorDeveloper): document the exact shape this connector expects, e.g. - /// "https://api.example.com" or "postgres://user:pass@host:5432/db". + /// "" or "postgres://user:pass@host:5432/db". /// `SecretString` because DSNs commonly embed credentials — never plain /// `String` for this field, see `PostgresSinkConfig::connection_string` /// in `sinks/postgres_sink` for the same pattern. From 162a1f1cd0986968372cd4c5dafd97d66377510b Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 12:55:57 +0530 Subject: [PATCH 08/10] update lib.rs to retrigger prechecks build --- core/connectors/sources/source_template/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 978811e3d1..0b6af85715 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -45,8 +45,8 @@ use async_trait::async_trait; use iggy_connector_sdk::retry::{ - CircuitBreaker, ConnectivityConfig, build_retry_client, check_connectivity_with_retry, - parse_duration, + CircuitBreaker, ConnectivityConfig, build_retry_client, + check_connectivity_with_retry, parse_duration, }; use iggy_connector_sdk::{ ConnectorState, Error, ProducedMessage, ProducedMessages, Schema, Source, From 1d5d2809e438a8bd95f341d849b630760cdfe257 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 13:30:31 +0530 Subject: [PATCH 09/10] Removing trailing space --- core/connectors/sources/source_template/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 0b6af85715..7421799d31 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -45,7 +45,7 @@ use async_trait::async_trait; use iggy_connector_sdk::retry::{ - CircuitBreaker, ConnectivityConfig, build_retry_client, + CircuitBreaker, ConnectivityConfig, build_retry_client, check_connectivity_with_retry, parse_duration, }; use iggy_connector_sdk::{ From e63f9e88134aac1fdb71a3994263ac22a91462a9 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sat, 29 Aug 2026 13:32:46 +0530 Subject: [PATCH 10/10] Update lib.rs --- core/connectors/sources/source_template/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/connectors/sources/source_template/src/lib.rs b/core/connectors/sources/source_template/src/lib.rs index 7421799d31..978811e3d1 100644 --- a/core/connectors/sources/source_template/src/lib.rs +++ b/core/connectors/sources/source_template/src/lib.rs @@ -45,8 +45,8 @@ use async_trait::async_trait; use iggy_connector_sdk::retry::{ - CircuitBreaker, ConnectivityConfig, build_retry_client, - check_connectivity_with_retry, parse_duration, + CircuitBreaker, ConnectivityConfig, build_retry_client, check_connectivity_with_retry, + parse_duration, }; use iggy_connector_sdk::{ ConnectorState, Error, ProducedMessage, ProducedMessages, Schema, Source,