hubcio commented on code in PR #3729:
URL: https://github.com/apache/iggy/pull/3729#discussion_r3636828435
##########
core/integration/src/harness/handle/connectors_runtime.rs:
##########
@@ -243,19 +243,33 @@ impl IggyServerDependent for ConnectorsRuntimeHandle {
}
async fn wait_ready(&mut self) -> Result<(), TestBinaryError> {
- let http_address = self.http_url();
+ // Prefer /health over `/` so readiness means the connectors API is up,
+ // not some other listener that happened to bind the reserved port.
+ let health_url = format!("{}/health", self.http_url());
let client = reqwest::Client::new();
Review Comment:
`reqwest::Client::new()` has no request timeout, so a black-holed GET
(connection accepted, no response) hangs `send()` forever - the loop never gets
back to the pid check above and the retry budget never fires. low risk on
localhost (dead process means ECONNREFUSED, which fast-fails), but a 1-2s
timeout like the fixture probe client makes both guards effective.
##########
core/connectors/sinks/elasticsearch_sink/src/lib.rs:
##########
@@ -83,7 +86,15 @@ impl ElasticsearchSink {
.map_err(|error| Error::Connection(format!("Invalid Elasticsearch
URL: {error}")))?;
let conn_pool =
elasticsearch::http::transport::SingleNodeConnectionPool::new(url);
- let mut transport_builder = TransportBuilder::new(conn_pool);
+ // elasticsearch-rs defaults to no timeout; without this, a hung ES
+ // connection blocks FFI open() and the connectors HTTP health never
binds.
+ let timeout_seconds = self
+ .config
+ .timeout_seconds
+ .unwrap_or(DEFAULT_TIMEOUT_SECONDS)
+ .max(1);
+ let mut transport_builder =
+
TransportBuilder::new(conn_pool).timeout(Duration::from_secs(timeout_seconds));
Review Comment:
this timeout is client-global in elasticsearch-rs, so it bounds bulk
requests in `consume()` too, not just `open()`. that changes the failure mode:
before, a bulk on degraded ES stalled until it eventually succeeded; now
anything over `timeout_seconds` returns `Err`, which the runtime discards
(#2927) with the offset already committed at poll (#2928) - the batch is
silently dropped. mechanism is pre-existing and tracked there, but this PR is
what arms it for slow-ES bulk. worth an upgrade note in the PR description:
raise `timeout_seconds` for slow bulk workloads, and note the tradeoff until
#2927/#2928 land.
separately, the comment slightly oversells the timeout as the flake fix -
with the 30s default the harness readiness budget (~20s) expires first, so what
fixes #3728 is the fixture readiness gate; this is the infinite-hang backstop.
maybe say that instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
