Skip to content

fix(connectors): Harden Elasticsearch sink test readiness#3729

Open
ryerraguntla wants to merge 8 commits into
apache:masterfrom
ryerraguntla:fix/elasticsearch-sink-test-flakiness
Open

fix(connectors): Harden Elasticsearch sink test readiness#3729
ryerraguntla wants to merge 8 commits into
apache:masterfrom
ryerraguntla:fix/elasticsearch-sink-test-flakiness

Conversation

@ryerraguntla

@ryerraguntla ryerraguntla commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes flaky Elasticsearch sink integration test where iggy-connectors
    never becomes healthy because sink open() can hang on Elasticsearch HTTP
    with no client timeout, while the connectors HTTP API only binds after
    plugin open.
  • Hardens the Elasticsearch Testcontainers fixture: use 127.0.0.1, always
    re-check /_cluster/health on reuse attach, self-heal a wedged
    iggy-test-elasticsearch container only, and sweep stale harness indices
    older than 30 minutes.
  • Improves harness readiness: poll connectors /health and fail fast if the
    child process dies during retries; refresh the index before document-count
    polls in the sink fixture.

Closes #3728

Test plan

  • cargo fmt --all
  • cargo sort --no-format --workspace
  • cargo clippy -p iggy_connector_elasticsearch_sink -p integration --all-features --all-targets -- -D warnings
  • cargo test -p integration -- connectors::elasticsearch::elasticsearch_sink::elasticsearch_sink_stores_json_messages
  • Confirm docker rm -f recovery targets only iggy-test-elasticsearch
    (no other Elasticsearch containers)
  • Optional: leave a reused iggy-test-elasticsearch running and re-run
    the sink test to verify reuse + readiness path

Notes

  • Container force-remove uses the fixed harness name
    iggy-test-elasticsearch only.
  • Stale index sweep is best-effort and age-gated so concurrent harness
    tests sharing the reused container are not disrupted.

Add a timeout to the Elasticsearch sink transport, make connectors-runtime readiness check `/health`, and strengthen the Elasticsearch test fixture startup path with retry, cluster-health polling, and stale-index cleanup. Also refresh the sink index before counting documents so test assertions see newly indexed data sooner.
Use short-timeout probes without retries for cluster health and stale
index sweep, and cap docker rm so a wedged daemon cannot stall setup.
@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Jul 22, 2026
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.25%. Comparing base (a5001d3) to head (dbf3ef7).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #3729       +/-   ##
=============================================
- Coverage     74.34%   40.25%   -34.10%     
  Complexity      950      950               
=============================================
  Files          1303     1303               
  Lines        148712   130071    -18641     
  Branches     124225   105584    -18641     
=============================================
- Hits         110562    52354    -58208     
- Misses        34673    74946    +40273     
+ Partials       3477     2771      -706     
Components Coverage Δ
Rust Core 32.13% <100.00%> (-42.45%) ⬇️
Java SDK 62.64% <ø> (ø)
C# SDK 72.28% <ø> (ø)
Python SDK 92.27% <ø> (ø)
PHP SDK 84.52% <ø> (ø)
Node SDK 92.70% <ø> (ø)
Go SDK 43.08% <ø> (ø)
Files with missing lines Coverage Δ
...ore/connectors/sinks/elasticsearch_sink/src/lib.rs 67.60% <100.00%> (+1.42%) ⬆️

... and 427 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ryerraguntla ryerraguntla changed the title fix(connectors): harden Elasticsearch sink test readiness fix(connectors): Harden Elasticsearch sink test readiness Jul 22, 2026
@ryerraguntla

Copy link
Copy Markdown
Contributor Author

/request-review @user-or-team

@slbotbm slbotbm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start() force-removes the shared container after any try_start() error, not only after confirming it is wedged. Transient health, port-inspection, or Docker errors can therefore remove an Elasticsearch container still used by another process. The fixed name limits which container is removed but does not provide ownership or concurrency safety. Could this recovery path verify the container is unhealthy and coordinate removal with a cross-process lock before running docker rm -f?

@ryerraguntla ryerraguntla Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add the logic to remove when required.

@ryerraguntla ryerraguntla Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dd2c04c

@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jul 22, 2026
Only docker rm the shared reuse container after inspect shows a removable
state, and serialize recovery with a cross-process lock so transient
start flakes cannot yank a healthy instance from a peer nextest worker.
@ryerraguntla

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Jul 23, 2026

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not in this diff, same class of problem:

  • elasticsearch source create_client() (core/connectors/sources/elasticsearch_source/src/lib.rs:304) still builds its transport with no timeout - same open()-hang failure mode this PR fixes for the sink, so source tests keep the #3728 flake class. worth a follow-up.
  • mcp.rs wait_ready() still uses the pattern this PR fixed for connectors: GET /, any Ok(_) counts as ready, no process-crash fast-fail. consistency follow-up.

.unwrap_or(DEFAULT_TIMEOUT_SECONDS)
.max(1);
let mut transport_builder =
TransportBuilder::new(conn_pool).timeout(Duration::from_secs(timeout_seconds));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread core/connectors/sinks/elasticsearch_sink/src/lib.rs
Comment thread core/integration/src/harness/handle/connectors_runtime.rs Outdated
Comment thread core/integration/tests/connectors/fixtures/elasticsearch/container.rs Outdated
Comment thread core/integration/tests/connectors/fixtures/elasticsearch/container.rs Outdated
Comment thread core/integration/tests/connectors/fixtures/elasticsearch/container.rs Outdated
Comment thread core/integration/tests/connectors/fixtures/elasticsearch/sink.rs Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jul 23, 2026
short-timeout health clients, wait_for_status , cluster readiness, async try_lock recovery, and timeout docs clarifying
client-global clamp vs apache#3728 flake fix.
@ryerraguntla

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review PR is waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Elasticsearch sink integration test flakes when connectors health never becomes ready

3 participants