You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "ryankert01 (via GitHub)" <gi...@apache.org> on 2026/10/15 11:16:14 UTC
[PR] test(integration): drop the elasticsearch nextest test-group [iggy]
ryankert01 opened a new pull request, #4555:
URL: https://github.com/apache/iggy/pull/4555
## Which issue does this PR address?
Closes #4544
## Rationale
The Elasticsearch test-group serialized the tests to avoid a Docker 409 race that the fixture already retries. It costs wall-clock time and its comment no longer matches the code.
## What changed?
The Elasticsearch connector tests ran one at a time because of a nextest test-group, although the fixture already retries the 409 and attaches to the winner's container. The group is removed, the same way it was [for Doris](https://github.com/apache/iggy/commit/d693eb93d878a7759221fba05dcea20e52dd1249).
Running in parallel exposes one gap the group hid: attaching to the reused container skips testcontainers' startup wait, so an attaching test had only 60s of [readiness budget](https://github.com/apache/iggy/blob/6e422363884305b77bb46123e3f0583c1e8a789c/core/integration/tests/connectors/fixtures/elasticsearch/container.rs#L43-L48) against the creator's 120s. Both now share that one 120s constant. The [recovery path](https://github.com/apache/iggy/blob/6e422363884305b77bb46123e3f0583c1e8a789c/core/integration/tests/connectors/fixtures/elasticsearch/container.rs#L484-L504) never removes a running container, so a peer that is still booting ES is safe.
The Doris fixture gets the same [120s startup timeout](https://github.com/apache/iggy/blob/6e422363884305b77bb46123e3f0583c1e8a789c/core/integration/tests/connectors/fixtures/doris/container.rs#L247) and a consistent `DorisContainer` error label.
## Local Execution
- Passed: `fmt`, `clippy -D warnings` on `integration`, the Elasticsearch suite in parallel 10 times (5 from a cold container, 5 warm), 9/9 each, the last 7 with retries off. Cold run: ~9.7s parallel vs 27.3s serial.
- Not run: the Doris tests. Its all-in-one image does not run on macOS. The Doris change compiles and passes clippy.
- `prek` ran, all hooks pass.
## AI Usage
- Claude Code.
- Implementation, local test runs, and a multi-agent review of the diff.
- The local runs above, and I read the testcontainers 0.27.3 reuse path to confirm that attachers skip the startup wait.
- Yes.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6193087607
@ryankert01 i'll investigate why this test fails:
` TRY 4 FAIL [ 86.737s] (2795/2795) integration::mod server::cg::multiple_clients::quic_heartbeat_enabled_true_heartbeat_interval_60s_consumer_group_rebalancing_timeout_1s`
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6191427536
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/4555?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 89.47%. Comparing base ([`ec985b7`](https://app.codecov.io/gh/apache/iggy/commit/ec985b7e005426455eed4de8f4653148cf5c5d90?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ac0439f`](https://app.codecov.io/gh/apache/iggy/commit/ac0439fd01160eb4881a7d18fc7a7a55001f9755?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4555 +/- ##
============================================
- Coverage 89.49% 89.47% -0.03%
Complexity 1735 1735
============================================
Files 1329 1329
Lines 284318 284318
Branches 242844 242844
============================================
- Hits 254448 254383 -65
- Misses 24227 24268 +41
- Partials 5643 5667 +24
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.72% <ø> (-0.03%)` | :arrow_down: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.25% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.38% <ø> (+0.01%)` | :arrow_up: |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.43% <ø> (ø)` | |
| [PHP SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.84% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `95.12% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.91% <ø> (+0.02%)` | :arrow_up: |
| [C++ SDK](https://app.codecov.io/gh/apache/iggy/pull/4555/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.44% <ø> (ø)` | |
[see 20 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/4555/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6436284032
/request-review @hubcio
/request-review @slbotbm
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on code in PR #4555:
URL: https://github.com/apache/iggy/pull/4555#discussion_r4283132224
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -42,7 +42,10 @@ const ELASTICSEARCH_HEALTH_ENDPOINT: &str = "/_cluster/health";
const ELASTICSEARCH_CONTAINER_NAME: &str = "iggy-test-elasticsearch";
// Short probe timeouts: create_http_client() uses 30s + retries and must not
// be used for readiness. One hung attempt there looks like a 60s+ test hang.
-const CLUSTER_READY_TIMEOUT_SECS: u64 = 60;
+// The overall budget doubles as the creator's startup timeout because
+// attaching to the reused container skips testcontainers' wait, so a test that
+// attaches while another one is still booting ES waits here instead.
+const CLUSTER_READY_TIMEOUT_SECS: u64 = 120;
Review Comment:
Agreed, but I'd rather not raise the kill. Each `try_start` now shares one 120s deadline across create and readiness, so a failing `start()` ends ~274s, under the 300s kill. Fixed.
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -42,7 +42,10 @@ const ELASTICSEARCH_HEALTH_ENDPOINT: &str = "/_cluster/health";
const ELASTICSEARCH_CONTAINER_NAME: &str = "iggy-test-elasticsearch";
// Short probe timeouts: create_http_client() uses 30s + retries and must not
// be used for readiness. One hung attempt there looks like a 60s+ test hang.
-const CLUSTER_READY_TIMEOUT_SECS: u64 = 60;
+// The overall budget doubles as the creator's startup timeout because
Review Comment:
Not reachable, I think: only the lock holder removes, and a peer's `created` container only exists for ms right after a create, which the holder's own attempt would have attached to.
##########
core/integration/tests/connectors/fixtures/doris/container.rs:
##########
@@ -244,6 +244,7 @@ async fn start_shared_container(
.with_port(FE_HTTP_PORT.tcp())
.with_expected_status_code(200u16),
))
+ .with_startup_timeout(Duration::from_secs(120))
Review Comment:
Fixed.
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -144,7 +147,7 @@ async fn start_shared_container() -> Result<ContainerAsync<GenericImage>, TestBi
.with_port(ELASTICSEARCH_PORT.tcp())
.with_expected_status_code(200u16),
))
- .with_startup_timeout(Duration::from_secs(120))
+ .with_startup_timeout(Duration::from_secs(CLUSTER_READY_TIMEOUT_SECS))
Review Comment:
Agreed, but it's out of scope here. I'll open a follow-up issue.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6190773265
<!-- iggy-pr-triage-welcome -->
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
- `/pin` - exempt the PR from the stale bot, `/unpin` to undo
See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md#pr-triage-commands) for details.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6191337217
/skill team-review-slim
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #4555:
URL: https://github.com/apache/iggy/pull/4555#discussion_r4282664269
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -42,7 +42,10 @@ const ELASTICSEARCH_HEALTH_ENDPOINT: &str = "/_cluster/health";
const ELASTICSEARCH_CONTAINER_NAME: &str = "iggy-test-elasticsearch";
// Short probe timeouts: create_http_client() uses 30s + retries and must not
// be used for readiness. One hung attempt there looks like a 60s+ test hang.
-const CLUSTER_READY_TIMEOUT_SECS: u64 = 60;
+// The overall budget doubles as the creator's startup timeout because
+// attaching to the reused container skips testcontainers' wait, so a test that
+// attaches while another one is still booting ES waits here instead.
+const CLUSTER_READY_TIMEOUT_SECS: u64 = 120;
Review Comment:
warning: `CLUSTER_READY_TIMEOUT_SECS` feeds both the create wait and the readiness deadline, and `start()` retries three times, so a failing setup can run about 410 s. That is past nextest's 300 s kill at `.config/nextest.toml:82`, before the fixture error prints. Add a `slow-timeout` override for these tests, as `chaos_monkey` has at `.config/nextest.toml:76`.
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -42,7 +42,10 @@ const ELASTICSEARCH_HEALTH_ENDPOINT: &str = "/_cluster/health";
const ELASTICSEARCH_CONTAINER_NAME: &str = "iggy-test-elasticsearch";
// Short probe timeouts: create_http_client() uses 30s + retries and must not
// be used for readiness. One hung attempt there looks like a 60s+ test hang.
-const CLUSTER_READY_TIMEOUT_SECS: u64 = 60;
+// The overall budget doubles as the creator's startup timeout because
Review Comment:
warning: `container_is_removable_wedged()` treats `created` as safe to remove, and `docker create` leaves a peer's container in that state between create and start. With the test group gone, a test in the recovery path can delete a peer's fresh container. Remove a `created` container only when `docker inspect` shows it older than a minute.
##########
core/integration/tests/connectors/fixtures/elasticsearch/container.rs:
##########
@@ -144,7 +147,7 @@ async fn start_shared_container() -> Result<ContainerAsync<GenericImage>, TestBi
.with_port(ELASTICSEARCH_PORT.tcp())
.with_expected_status_code(200u16),
))
- .with_startup_timeout(Duration::from_secs(120))
+ .with_startup_timeout(Duration::from_secs(CLUSTER_READY_TIMEOUT_SECS))
Review Comment:
simplification: `start_shared_container` here and `doris/container.rs:220` are the same create-or-attach loop, down to the same 30-attempt retry and `is already in use` check. Move it into one helper in `fixtures/mod.rs` that takes the per-fixture request builder, because both call sites already rebuild the request per attempt.
##########
core/integration/tests/connectors/fixtures/doris/container.rs:
##########
@@ -244,6 +244,7 @@ async fn start_shared_container(
.with_port(FE_HTTP_PORT.tcp())
.with_expected_status_code(200u16),
))
+ .with_startup_timeout(Duration::from_secs(120))
Review Comment:
nit: `.with_startup_timeout(Duration::from_secs(120))` leaves 120 unnamed, while the elasticsearch fixture names the same value in `CLUSTER_READY_TIMEOUT_SECS`. Add a named constant next to `DEFAULT_POLL_ATTEMPTS`.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio merged PR #4555:
URL: https://github.com/apache/iggy/pull/4555
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6190842348
`/skill` is limited to repository committers, and `ryankert01` does not have write access.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6190823879
/skill team-review-slim
just want to test it out.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6192469781
/ready
*it's an interesting bot.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(integration): drop the elasticsearch nextest test-group [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on PR #4555:
URL: https://github.com/apache/iggy/pull/4555#issuecomment-6381421292
@ryankert01 could you rebase and see if the pr hubert opened solves your issue?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org