feat(connectors): add generic JDBC source connector#3588
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3588 +/- ##
============================================
- Coverage 74.13% 74.11% -0.02%
Complexity 937 937
============================================
Files 1258 1258
Lines 131452 131450 -2
Branches 107320 107363 +43
============================================
- Hits 97455 97429 -26
+ Misses 30909 30892 -17
- Partials 3088 3129 +41
🚀 New features to boost your workflow:
|
80435e8 to
9e0c750
Compare
| query = query.replace("{last_offset}", "e_sql_literal(offset)); | ||
| } else if let Some(ref initial) = self.config.initial_offset { | ||
| query = query.replace("{last_offset}", "e_sql_literal(initial)); | ||
| } else { |
There was a problem hiding this comment.
{tracking_column} never substituted in build_query; only {last_offset} replaced. README/examples use WHERE {tracking_column} > {last_offset} → invalid SQL at runtime. Fix: substitute from config.tracking_column or fix docs.
| } | ||
|
|
||
| /// Create HikariCP connection pool | ||
| fn create_connection_pool_internal(&self, env: &mut JNIEnv) -> Result<GlobalRef, Error> { |
There was a problem hiding this comment.
Two places - 472-485,1255 enable_connection_pool loads HikariCP via JNI but JVM classpath = single driver_jar_path only → pool mode fails unless HikariCP bundled. Fix: add HAR to classpath, or disable pool flag until wired.
| let state = self.state.lock().expect("state mutex poisoned"); | ||
| self.build_query(&state) | ||
| }; | ||
| info!("Executing query: {}", query); |
There was a problem hiding this comment.
info!("Executing query: {}", query) logs substituted offset/filters. Fix: debug! or redact.
|
/author |
Sure, closed the parent PR with a note mentioning two different PR approach for source and sink. |
9966922 to
6d7c88b
Compare
|
/ready |
| } | ||
|
|
||
| /// Best-effort `Connection.isValid(timeout)` check. Returns false on any | ||
| /// JNI error so the caller re-establishes the connection. |
There was a problem hiding this comment.
lib.rs:473-478, 551-560,634-988 — JNI exceptions left uncleared on ~45 of 46 fallible call_method/get_string sites (only classify_query_failure clears, 2 sites). Sharpest instance: statement.close() at 557 fires while executeQuery's SQLException is still pending, cleared one line too late. JNI spec forbids further JNI calls while an exception is pending — can abort the whole embedded JVM, i.e. the entire connectors-runtime process, not just this connector. Fix: route every fallible JNI call through an exception-clearing helper.
| /// | ||
| /// The mutex is held only briefly: once to read the current offset for | ||
| /// query building, and once after the JNI work to write the updated state. | ||
| fn execute_query(&self, env: &mut JNIEnv) -> Result<Vec<ProducedMessage>, Error> { |
There was a problem hiding this comment.
lib.rs:484-513 vs runtime/src/source.rs:456-517 — state.last_offset advances in-memory as soon as rows are fetched, before delivery is confirmed. Disk-state persist is correctly gated on send
success, but the in-memory offset never rolls back on a transient send failure — that batch is silently and permanently skipped, and the next successful poll's further-advanced offset overwrites disk state,
erasing any replay chance. Breaks the connector's own at-least-once promise on ordinary non-crash failures. Fix: derive next offset from last durably-persisted state, not from a struct field advanced ahead of
confirmed delivery.
There was a problem hiding this comment.
This is the same model the existing offset-tracking sources use (postgres_source advances state.tracking_offsets in poll() before delivery too), so the durable guarantee today is at-least-once across restarts but unable to support transient in-process send failure without a restart.
IMO the clean fix can't be done at the connector layer, because the runtime owns the state file and hands the connector its state only once at open(), with no per-poll delivery ack to roll the in-memory offset back. I documented the exact guarantee in the README and would like to track the stronger guarantee as a runtime-level follow-up (roll back / re-feed the offset on send failure), which benefits every offset-tracking source uniformly.
Happy to open that issue. Please let me know your thoughts.
| Ok(s) => s, | ||
| Err(_) => return Err(classify_query_failure(env, "prepare statement")), | ||
| }; | ||
|
|
There was a problem hiding this comment.
lib.rs:539-549,743-745 — bulk mode has no pagination beyond setMaxRows; tables larger than batch_size never fully sync, silently. Fix: log a loud warning when a poll returns exactly batch_size rows so truncation is diagnosable; full cross-DB OFFSET pagination is a larger follow-up, scope separately. Also fix the Mode::Bulk doc comment and README, both currently claim "full table scan" unconditionally.
| info!("JDBC source connector [{}] opened successfully", self.id); | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
lib.rs:1028-1055 — blocking JNI/JDBC I/O runs directly on the shared async runtime; chains with the mutex-poisoning finding since the operator restart path also runs on that same runtime with no timeout of its own. Fix: wrap the fetch body of poll() in tokio::task::block_in_place (works today, no SDK change needed since the runtime is already multi-thread); apply the same wrapping to close().
|
|
||
| /// Return the larger of two offset values. Both are compared numerically when | ||
| /// they parse as numbers (covers integer and decimal tracking columns), and | ||
| /// lexicographically otherwise (covers ISO-8601 timestamps and text keys). |
There was a problem hiding this comment.
lib.rs:1171-1178 — larger_offset compares via f64, loses precision past 2^53, contradicts the file's own NUMERIC/DECIMAL-as-string rationale. Fix: try i128 parse/compare first (exact for the full BIGINT range), fall back to f64 only for genuinely fractional values, lexical for non-numeric.
|
|
||
| /// Quote a value as a SQL string literal, escaping embedded single quotes by | ||
| /// doubling them. Used to substitute the incremental `{last_offset}` value, | ||
| /// which originates from a (DB-controlled) tracking-column value. |
There was a problem hiding this comment.
lib.rs:1148-1150 — quote_sql_literal doesn't escape backslash, MySQL default sql_mode breakout possible (downgraded from critical, requires attacker already has DB write access). Fix: escape backslash
before doubling quotes; real long-term fix is binding the offset as a PreparedStatement parameter instead of string substitution — scope that as a follow-up refactor.
| } | ||
| }; | ||
|
|
||
| let columns = self.read_column_metadata(env, &result_set)?; |
There was a problem hiding this comment.
lib.rs:562-569 — statement.close() skipped on read-error path, leaks PreparedStatement/cursor. Fix: split the try-body into its own function and close unconditionally in the caller after it returns, on both Ok and Err.
| "Creating direct JDBC connection to: {}", | ||
| sanitize_jdbc_url(self.config.jdbc_url.expose_secret()) | ||
| ); | ||
| let conn = self.create_direct_connection_internal(&mut env)?; |
There was a problem hiding this comment.
lib.rs:322,441,452,458,461,489,500,1046,1075,1120 — mutex .expect()-on-poison cluster, no self-heal, contradicts repo's no-unwrap/expect rule. Fix: replace .expect() with a helper that maps PoisonError
to a returned Error::Connection/Error::InitError instead of panicking, so a poisoned lock surfaces as a logged failure rather than a permanent panic loop.
|
/author |
|
Thanks for the thorough review. I have pushed a revision that addresses the feedback. |
|
/ready |
| .or(self.config.initial_offset.as_deref()); | ||
|
|
||
| // Without an offset yet, drop the incremental predicate but keep the rest | ||
| // of the query (e.g. an ORDER BY) intact. |
There was a problem hiding this comment.
899-900 + :647 + :608 — cold-start incremental drops WHERE (no offset), setMaxRows caps batch, advances max_offset → next poll > max permanently skips unread lower keys when no ORDER BY. Same class any truncated unordered poll. Fix: require ORDER BY {tracking_column} at open (or forbid cold-start without initial_offset + ordered paging). README :208-209 claims order optional = false.
| /// Validate configuration before touching the JVM or the database, so bad | ||
| /// config surfaces immediately at `open()` with an actionable message rather | ||
| /// than as an opaque wrapped JVM error or only after the first poll sleep. | ||
| fn validate_config(&self) -> Result<(), Error> { |
There was a problem hiding this comment.
lib.rs:1174-1198 + README :193 — incremental does not require tracking_column → offset never advances → infinite re-read. Fix: fail open() if mode=incremental && no tracking_column; README Required.
|
|
||
| // Update state with results (short lock) | ||
| { | ||
| let mut state = lock_mutex(&self.state, "state")?; |
There was a problem hiding this comment.
lib.rs:608-610 + runtime/src/source.rs:464-515 — in-memory offset before send; disk only after send OK → in-process send fail skips batch. README honest; postgres pattern. Fix: defer mem advance / rollback (follow-up OK).
| // full batch there is normal paging that the next poll advances past, not | ||
| // truncation. Full cross-database OFFSET pagination is a separate | ||
| // follow-up. | ||
| if self.config.mode == Mode::Bulk && row_count == self.config.batch_size as u64 { |
There was a problem hiding this comment.
lib.rs:597-603 — bulk setMaxRows truncates; surplus never synced. Fix: paginate or fail-closed.
| } | ||
| } | ||
|
|
||
| /// Extract the tracking-column value as a string offset, or `None` when it is |
There was a problem hiding this comment.
lib.rs:1152-1158 — NULL tracking skipped for watermark; row emitted; offset jumps → never re-fetch. Fix: reject NULL track / IS NOT NULL.
| row_data.insert(final_col_name.clone(), value); | ||
|
|
||
| // Track offset if this is the tracking column | ||
| if let Some(ref tracking_col) = self.config.tracking_column |
There was a problem hiding this comment.
lib.rs:841-844 — offset match raw JDBC col_name not snake final_col_name → miss → dups. Fix: match normalized name.
|
|
||
| [package] | ||
| name = "iggy_connector_jdbc_source" | ||
| version = "0.1.0" |
There was a problem hiding this comment.
Cargo.toml:20,32 + no publish — 0.1.0 / rlib vs siblings 0.4.1-edge.1 / lib + publish=false. Fix: align.
| # Required by source_connector! macro | ||
| dashmap = { workspace = true } | ||
|
|
||
| # For parsing duration strings |
There was a problem hiding this comment.
humantime-serde = "1.1" direct pin; siblings use workspace humantime. Fix: drop; parse like postgres_source.
| Ordering the query by the tracking column is still recommended for readability. | ||
|
|
||
| **Example:** | ||
|
|
There was a problem hiding this comment.
README/examples partition_id dead field; Oracle ROWNUM > {last_offset} wrong cursor. Fix: strip / use ID timestamp.
|
|
||
| let now_ms = now.timestamp_millis() as u64; | ||
| Ok(ProducedMessage { | ||
| id: Some(Uuid::new_v4().as_u128()), |
There was a problem hiding this comment.
lib.rs:873 — Uuid::new_v4() → at-least-once not idempotent by msg id. Matches postgres_source. Fix: deterministic id (follow-up).
| use java::sql::Types; | ||
|
|
||
| // Check if null first | ||
| let obj = jni!( |
There was a problem hiding this comment.
double JNI getObject+getter (:989-1017); isValid up to 30s on block_in_place worker (:557). Fix: wasNull+one getter; short liveness timeout.
|
Still there are few more around the order by clause . Check with what is mentioned in README and the actual implementation. |
|
/author |
Adds a JDBC source connector that polls a configured SQL query against any JDBC-compliant database (PostgreSQL, MySQL, Oracle, SQL Server, H2) through an embedded JVM and produces each row as a JSON message. Supports bulk and incremental (offset-tracked) modes with persisted offset state. Also adds the shared connector integration-test harness and a small TCP listener change so that harness can read the server's written runtime config when bound to a fixed port. The matching JDBC sink connector follows in a separate PR.
Aligns the JDBC source crate with the other source connectors: crate version/type and publish flag, and poll_interval parsing via the workspace humantime crate rather than a direct humantime-serde pin. Removes a redundant per-column JNI getObject probe (using wasNull after the typed getter instead) and caps the per-poll liveness check at five seconds so a dead connection cannot stall a shared worker. Drops the unused partition_id field from the example configs and docs.
|
/ready |
Which issue does this PR address?
Relates to #2500
(This is the first of two PRs splitting #2500 for easier review: the JDBC source connector here, and the JDBC sink connector as a follow-up PR once this lands. This PR also carries the shared connector test harness and a small server-side fix that both connectors' integration tests rely on.)
Rationale
Iggy only shipped PostgreSQL-specific connectors, so every other database needed a purpose-built connector. A generic JDBC source lets Iggy read from any JDBC-compliant database (PostgreSQL, MySQL, Oracle, SQL Server, H2, and others) through its existing JDBC driver, instead of writing and maintaining a new connector per database.
What changed?
Adds a JDBC source connector that talks to any JDBC database through an embedded JVM over JNI, using the standard
java.sqlAPI and the database's own driver JAR. It polls a configured SQL query and produces each row as a JSON message, in bulk mode (re-reads the query result each poll, capped at batch_size) or incremental mode (tracks a {last_offset} placeholder and persists the offset across restarts). Column values are mapped to JSON with type fidelity in mind (decimals as strings to avoid f64 precision loss, binary as base64), and query failures are classified into transient vs permanent error variants by SQLState, which sets the returned error variant and the log message. It does not currently change the connector's retry or backoff behavior.How to review
core/connectors/sources/jdbc_source/src/lib.rsopen()(JVM init, driver load, connection) thenpoll()thenbuild_query(offset substitution viaquote_sql_literal) thenread_rows/extract_column_value(JDBC to JSON) thenbuild_message(offset state persistence)get_or_create_jvm,get_connection,read_rowspush_local_frame/pop_local_frameso large result sets do not overflow the JNI localisValidreconnect, pooled-connection returnextract_column_valueNUMERIC/DECIMALas string, binary as base64,BIGINTcaveat documented in the READMEis_transient_sql_state,classify_query_failure,take_pending_sql_exception08/40/53/57/58are transient; pending Java exceptions are clearedDebugimpl, the secret serde modules,sanitize_jdbc_urljdbc_urlandpasswordareSecretString, never logged or serializedcore/integration/tests/connectors/mod.rssetup_runtimeand theConnectorsRuntime/ConnectorsIggyClienthelpers used by the JDBC (and future connector) testscore/server/src/tcp/tcp_listener.rs(3 lines)PortReserver) and waits for the server to writecurrent_config.tomlto learnport == 0), so a fixed-port server hung. The fix always notifies the config writer once the listener binds. TheSO_REUSEPORTcross-shard broadcast stays gated onport == 0, so multi-shard behaviour is unchanged. Worth a check for cluster mode.Config reference and per-database examples:
core/connectors/sources/jdbc_source/README.md, itsconfig.toml, andcore/connectors/runtime/example_config/connectors/jdbc_*.toml.Local Execution
cargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warnings,cargo check --all --all-features,cargo sort --check,cargo machete,license headers (
hawkeye check), andmarkdownlint.test time is about 20 to 25 seconds; unit tests are effectively instant.
iggy-server, created a stream and topic, and ran the JDBC source runtime against a Postgres container seeded with 150 rows. The source read all 150 rows in a single poll,advanced its incremental offset, and produced exactly 150 messages to the topic with no errors.
prekwas not run (not installed locally); the equivalent checks above were run manually.Limitations
AI Usage