Repository navigation
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 #3873 +/- ##
============================================
- Coverage 80.83% 80.82% -0.01%
+ Complexity 3107 3106 -1
============================================
Files 1467 1468 +1
Lines 356533 359013 +2480
Branches 300421 302902 +2481
============================================
+ Hits 288194 290178 +1984
- Misses 61617 62071 +454
- Partials 6722 6764 +42
🚀 New features to boost your workflow:
|
|
Getting some errors with a 503 returned when they passed locally, for example for Typo. Will try running again later: Connecting to github.com (github.com)|140.82.112.4|:443... connected.
HTTP request sent, awaiting response... 503 Service Unavailable
2026-08-12 19:30:09 ERROR 503: Service Unavailable.Update: These seem to have resolved. |
|
/ready |
| // The top-level flag lets a clean batch skip the per-item scan entirely. | ||
| if !response | ||
| .get("errors") | ||
| .and_then(Value::as_bool) | ||
| .unwrap_or(true) | ||
| { | ||
| return Ok(BulkAttempt { | ||
| indexed: items.len(), | ||
| ..BulkAttempt::default() |
There was a problem hiding this comment.
This fast path still accepts malformed item entries without validating them. For example, {"errors":false,"items":[{}]} passes the length check and is reported as one indexed document, even though the response contains no recognizable index result or successful status. That leaves the malformed-response fix incomplete, and the runtime will commit the offset for a document that was never actually accounted for. Please validate that every item contains the expected operation result and a 2xx status before returning success, and add an errors: false regression case with a malformed item.
|
|
||
| [package] | ||
| name = "iggy_connector_opensearch_sink" | ||
| version = "0.5.0-edge.1" |
| impl std::fmt::Debug for OpenSearchSink { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("OpenSearchSink") | ||
| .field("id", &self.id) | ||
| .field("config", &self.config) | ||
| .field("client", &self.client.is_some()) | ||
| .field("invocations_count", &self.invocations_count) | ||
| .field("documents_indexed", &self.documents_indexed) | ||
| .field("errors_count", &self.errors_count) | ||
| .finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
This hides the authenticated client and relies on SecretString for the dedicated password field, but it still prints self.config.url verbatim through self.config. Since embedded URL credentials are rejected only later in open(), formatting a newly constructed sink from https://admin:hunter2@host exposes hunter2. Let's redact or sanitize the URL in the resolved config's Debug output.
| secrecy = { workspace = true } | ||
| serde = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| simd-json = { workspace = true } |
There was a problem hiding this comment.
simd-json is used only by test fixtures inside the #[cfg(test)] module. It should be under [dev-dependencies].
db6f3e3 to
aedfdb1
Compare
# Conflicts: # Cargo.toml
|
|
/ready |
| }); | ||
|
|
||
| let rejected = retry_set.len(); | ||
| pending = retry_set; |
There was a problem hiding this comment.
critical: partial retries can overwrite a newer document with an older item that shares its ID. preserve source order across retries for the same ID.
| upsert_metadata_field(id, document, field, value); | ||
| } | ||
|
|
||
| if let Some(headers) = &message.headers |
There was a problem hiding this comment.
critical: a payload's iggy_headers survives when real headers are absent, so a scalar value can block later documents with headers. remove payload iggy_headers before adding metadata.
| // through `Serialize`, which only ever sees `&str` and so | ||
| // must copy `data` again even though it's already owned here. | ||
| Value::Object(Map::from_iter([ | ||
| ("data".to_string(), Value::String(data)), |
There was a problem hiding this comment.
critical: automatic date detection can map a header string as a date, then reject later documents with ordinary strings. explicitly map iggy_headers.*.data as text or keyword before the first write.
| // Connector-owned rather than exposing `opensearch::params::Refresh` directly: | ||
| // keeps this config's wire format under this connector's own control instead | ||
| // of an internal detail of the opensearch crate. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] |
There was a problem hiding this comment.
warning: refresh=true and refresh=false environment overrides become booleans, which RefreshPolicy rejects at startup. accept booleans alongside the documented strings.
| ); | ||
| } | ||
|
|
||
| if documents.is_empty() { |
There was a problem hiding this comment.
warning: consume() reports success after invalid document IDs drop records, so the runtime counts those records as processed without errors. return an error after valid documents finish if any record fails validation.
There was a problem hiding this comment.
Should we hold off on this until the runtime-level discussion in #2927 and #2928 settles? Dropping a bad record, counting it and continuing is the runtime's own convention for records that fail decoding or transforms, and the README documents it for this sink. Failing the batch wouldn't recover the records anyway, since offsets are committed at poll time.
| let mut retries = 0u32; | ||
|
|
||
| loop { | ||
| let request = client.bulk(BulkParts::None).body(vec![body.clone()]); |
There was a problem hiding this comment.
warning: Bulk::body wraps the cached bytes in NdBody, which copies the whole body on every request. pass Bytes directly to client.transport().send and preserve the request parameters.
|
|
||
| let rejected = retry_set.len(); | ||
| pending = retry_set; | ||
| body = build_bulk_body(&self.config.index, &pending)?; |
There was a problem hiding this comment.
warning: when every item returns 429, index_chunk() serializes the unchanged documents again. reuse the cached body unless the retry set removes documents.
| /// Out-of-range positions are dropped: a response carrying more items than | ||
| /// were sent would otherwise panic, and a panic crossing the plugin's | ||
| /// `extern "C"` boundary aborts the whole connectors runtime process. | ||
| fn documents_at<'a>( |
There was a problem hiding this comment.
simplification: documents_at() checks positions that its caller already bounds to the pending documents. select retry documents directly and remove the impossible-position test.
| let messages_count = messages.len(); | ||
| let mut documents = Vec::with_capacity(messages_count); | ||
| let mut invalid_records = 0usize; | ||
| let mut preparation_errors = 0usize; |
There was a problem hiding this comment.
simplification: prepare_document() only returns InvalidRecordValue, but the sink tracks separate preparation failures. use one counter and one error branch.
| assert_eq!(sink.errors_count.load(Ordering::Relaxed), 0); | ||
| } | ||
|
|
||
| async fn mock_health_sink(server: &MockServer, base_path: &str) -> OpenSearchSink { |
There was a problem hiding this comment.
simplification: mock_health_sink and mock_bulk_sink return futures even though they only do synchronous work. make both helpers synchronous and remove their callers' awaits.
|
/ready |
# Conflicts: # Cargo.lock # Cargo.toml # core/integration/tests/connectors/fixtures/mod.rs # core/integration/tests/connectors/mod.rs # scripts/bump-version.sh
Which issue does this PR address?
Closes #3504
Rationale
Iggy connectors ship sinks for several external systems but not for OpenSearch, a widely used search/analytics backend. This adds one, modeled on the existing
elasticsearch_sinkshape but closing a retry gap that sink still has (see Known trade-offs).What changed?
Adds
core/connectors/sinks/opensearch_sink/, following the sink lifecycle end to end:open(): validates config (URL shape, credential pairing,document_id_fieldconstraints), thenretry_on_open-wraps a cluster health check, an index-exists check, and, whencreate_index_if_not_exists(defaulttrue), index creation with an optional custom mapping. Capped atmax_open_retrieswith exponential backoff and jitter (sharedretryhelpers).consume(): batches incoming messages intobatch_sizechunks, builds aniggy_*-enriched document per message (hashed or field-derived_id), and hands each chunk toindex_chunk.index_chunk(): POSTs a_bulkrequest and loops up tomax_retrieswith the same backoff._bulkanswers200even when individual documents fail, so each response is parsed per item rather than trusting the top-level status: permanent failures (4xx, e.g. a mapping conflict) are recorded immediately, while transient ones (429/5xx) shrink the pending set to just the rejected documents and get resent, so a partial rejection under load doesn't re-index or lose the rest of the chunk. Counts from earlier attempts are merged into the outcome so a later failure doesn't erase already-indexed documents from the tally.close(): drops the client, no special teardown.Integration tests (
core/integration/tests/connectors/opensearch/) run against a real container (testcontainers-modules, reused across tests viaReuseDirective::Always, per-test-unique index names for isolation), covering the happy path plus a static mapping conflict, a missing index with index-creation disabled, and confirming a failing chunk doesn't block chunks queued behind it.Credentials: HTTP Basic auth only (
username/password, both-or-neither validated at config time);passwordis aSecretString, never logged or serialized. AWS SigV4 (AWS-managed OpenSearch / Serverless) is not supported.Known trade-offs, deliberately out of scope here, verified against current
master:elasticsearch_sinkhas the same gap this PR fixes for OpenSearch:bulk_index_documents(elasticsearch_sink/src/lib.rs:205-219) tallies per-item_bulkfailures intoerrors_countbut never retries the transient subset (e.g. 429es_rejected_execution_exception). Worth a follow-up issue rather than folding into this PR.consume()return value:core/connectors/runtime/src/sink.rs:740-748invokes the FFIconsumecallback as a bare statement, never binding itsi32result, soprocess_messagesalways returnsOk. Combined with offsets auto-committing at poll time (sink.rs:522), a plugin-level failure never reaches connector status,last_error, or/stats, and the batch is never redelivered. Pre-existing, repo-wide, affects every sink.meilisearch_sink(lib.rs:451-454) andelasticsearch_sink(lib.rs:319-328) both silently dropiggy_headers/_iggy_headers:BTreeMap<HeaderKey, HeaderValue>can't serialize as a JSON object (serde_jsonrequires string keys), and both sinks swallow that error viaif let Ok(...)instead of surfacing it.core/commoneven ships aserialize_headersworkaround for this exact case that neither sink uses.elasticsearch_source, the client is built onSingleNodeConnectionPool(lib.rs:208) with no cluster sniffing or multi-node failover. A dead configured node fails every request rather than routing around it.Local Execution
AI Usage