Skip to content

Remove bad test test_system_replicated_fetches#71071

Merged
alexey-milovidov merged 1 commit intomasterfrom
remove-bad-test-2
Oct 26, 2024
Merged

Remove bad test test_system_replicated_fetches#71071
alexey-milovidov merged 1 commit intomasterfrom
remove-bad-test-2

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Oct 25, 2024 •
edited
Loading

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

The test relies on a race condition - that the data part is large enough to be noticed in the system.replicated_fetches table in at least one of time points spread in 100 milliseconds. This is not guaranteed.

https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMSBNT05USAogICAgQU5EIGhlYWRfcmVmID0gJ21hc3RlcicKICAgIEFORCB0ZXN0X3N0YXR1cyAhPSAnU0tJUFBFRCcKICAgIEFORCAodGVzdF9zdGF0dXMgTElLRSAnRiUnIE9SIHRlc3Rfc3RhdHVzIExJS0UgJ0UlJykgCiAgICBBTkQgY2hlY2tfc3RhdHVzICE9ICdzdWNjZXNzJwogICAgQU5EIHBvc2l0aW9uKHRlc3RfbmFtZSwgJ3JlcGxpY2F0ZWRfZmV0Y2hlcycpID4gMApPUkRFUiBCWSBjaGVja19zdGFydF90aW1l

This closes #45435

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

  • Allow: All Required Checks
  • Allow: Stateless tests
  • Allow: Stateful tests
  • Allow: Integration Tests
  • Allow: Performance tests
  • Allow: All Builds
  • Allow: batch 1, 2 for multi-batch jobs
  • Allow: batch 3, 4, 5, 6 for multi-batch jobs
  • Exclude: Style check
  • Exclude: Fast test
  • Exclude: All with ASAN
  • Exclude: All with TSAN, MSAN, UBSAN, Coverage
  • Exclude: All with aarch64, release, debug
  • Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, etc.)
  • Exclude: AST fuzzers
  • Do not test
  • Woolen Wolfdog
  • Upload binaries for special builds
  • Disable merge-commit
  • Disable CI cache

@alexey-milovidov alexey-milovidov added the 🍃 green ci 🌿 Fixing flaky tests in CI label Oct 25, 2024
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 25, 2024
@robot-ch-test-poll4
Copy link
Copy Markdown
Contributor

robot-ch-test-poll4 commented Oct 25, 2024 •
edited by robot-ch-test-poll3
Loading

This is an automated comment for commit 878b1bc with description of existing statuses. It's updated for the latest CI running

✅ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
BuildsThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
ClickBenchRuns [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table✅ success
Compatibility checkChecks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help✅ success
Docker keeper imageThe check to build and optionally push the mentioned image to docker hub✅ success
Docker server imageThe check to build and optionally push the mentioned image to docker hub✅ success
Docs checkBuilds and tests the documentation✅ success
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
Flaky testsChecks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc✅ success
Install packagesChecks that the built packages are installable in a clear environment✅ success
Integration testsThe integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests✅ success
Performance ComparisonMeasure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests✅ success
Stateful testsRuns stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc✅ success
Stateless testsRuns stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc✅ success
Stress testRuns stateless functional tests concurrently from several clients to detect concurrency-related errors✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Unit testsRuns the unit tests for different release types✅ success
Upgrade checkRuns stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts✅ success

@Algunenano
Copy link
Copy Markdown
Member

Algunenano commented Oct 25, 2024

In this case at least the problem is not delays, the problem is that 2 parts were created and the first one was all_1_1_0 and not all_0_0_0. I don't think we have any guarantees about system.replicated_fetches order (certainly there is no ORDER BY).

Checking the node 2 logs it seems it downloads the all_1_1_0 part first too:

2024.09.26 15:19:32.629126 [ 605 ] {} <Test> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, reserving space using storage balanced reservation
2024.09.26 15:19:32.629158 [ 8 ] {} <Debug> TCPHandler: Connected ClickHouse client version 24.10.0, revision: 54471, user: default.
2024.09.26 15:19:32.629190 [ 605 ] {} <Test> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, reserving space using TTL rules
2024.09.26 15:19:32.629251 [ 605 ] {} <Trace> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68): Trying to reserve 188.35 MiB using storage policy from min volume index 0
2024.09.26 15:19:32.629299 [ 8 ] {} <Debug> TCP-Session: 11b6bf9c-a190-4943-9a72-546d536d1f70 Authenticating user 'default' from 172.16.1.1:58976
2024.09.26 15:19:32.629340 [ 605 ] {} <Trace> DiskLocal: Reserved 188.35 MiB on local disk `default`, having unreserved 100.14 GiB.
2024.09.26 15:19:32.629388 [ 605 ] {} <Trace> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, getting disk from reservation default with type 'local'
2024.09.26 15:19:32.629408 [ 8 ] {} <Debug> TCP-Session: 11b6bf9c-a190-4943-9a72-546d536d1f70 Authenticated with global context as user 94309d50-4f52-5250-31bd-74fecac179db
2024.09.26 15:19:32.629436 [ 8 ] {} <Debug> TCP-Session: 11b6bf9c-a190-4943-9a72-546d536d1f70 Creating session context with user_id: 94309d50-4f52-5250-31bd-74fecac179db
2024.09.26 15:19:32.629449 [ 605 ] {} <Debug> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Downloading part all_1_1_0 onto disk default.
2024.09.26 15:19:32.629602 [ 605 ] {} <Debug> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Downloading files 10
2024.09.26 15:19:32.629610 [ 609 ] {} <Test> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, reserving space using storage balanced reservation
2024.09.26 15:19:32.629647 [ 609 ] {} <Test> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, reserving space using TTL rules
2024.09.26 15:19:32.629698 [ 609 ] {} <Trace> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68): Trying to reserve 815.66 MiB using storage policy from min volume index 0
2024.09.26 15:19:32.629773 [ 609 ] {} <Trace> DiskLocal: Reserved 815.66 MiB on local disk `default`, having unreserved 99.96 GiB.
2024.09.26 15:19:32.629808 [ 609 ] {} <Trace> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Disk for fetch is not provided, getting disk from reservation default with type 'local'
2024.09.26 15:19:32.629850 [ 609 ] {} <Debug> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Downloading part all_0_0_0 onto disk default.
2024.09.26 15:19:32.629951 [ 609 ] {} <Debug> default.t (ead11d22-7547-4be2-b478-ec491f5ffa68) (Fetcher): Downloading files 10

I see different threads being used in parallel, so yeah, this test worked by pure luck most of the time.

@Algunenano Algunenano self-assigned this Oct 25, 2024
@Algunenano Algunenano self-requested a review October 25, 2024 20:33
@alexey-milovidov alexey-milovidov merged commit 15d6831 into master Oct 26, 2024
@alexey-milovidov alexey-milovidov deleted the remove-bad-test-2 branch October 26, 2024 18:58
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 26, 2024
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Nov 1, 2024
Merged
robot-ch-test-poll2 added a commit that referenced this pull request Nov 1, 2024
@robot-ch-test-poll2
Merge pull request #71386 from ClickHouse/cherrypick/24.10/15d683148d…
929903b 
…c95e8b727d1538e6d03ce41b38c4bf

Cherry pick #71071 to 24.10: Remove bad test `test_system_replicated_fetches`
robot-clickhouse added a commit that referenced this pull request Nov 1, 2024
@robot-clickhouse
Backport #71071 to 24.10: Remove bad test `test_system_replicated_fet…
0abeb22 
…ches`
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Nov 1, 2024
Merged
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created-cloud deprecated label, NOOP label Nov 1, 2024
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Nov 1, 2024
robot-clickhouse-ci-1 added a commit that referenced this pull request Nov 1, 2024
@robot-clickhouse-ci-1
Merge pull request #71387 from ClickHouse/backport/24.10/71071
d5af396 
Backport #71071 to 24.10: Remove bad test `test_system_replicated_fetches`
@Algunenano Algunenano added v24.9-must-backport and removed pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP v24.10-must-backport labels Nov 7, 2024
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Nov 7, 2024
Merged
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP labels Nov 8, 2024
@Algunenano Algunenano removed the pr-backports-created-cloud deprecated label, NOOP label Nov 11, 2024
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created-cloud deprecated label, NOOP label Nov 11, 2024
@Algunenano Algunenano removed the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Nov 11, 2024
robot-ch-test-poll2 added a commit that referenced this pull request Nov 11, 2024
@robot-ch-test-poll2
Merge pull request #71576 from ClickHouse/cherrypick/24.9/15d683148dc…
a2592c2 
…95e8b727d1538e6d03ce41b38c4bf

Cherry pick #71071 to 24.9: Remove bad test `test_system_replicated_fetches`
robot-clickhouse added a commit that referenced this pull request Nov 11, 2024
@robot-clickhouse
Backport #71071 to 24.9: Remove bad test `test_system_replicated_fetc…
3de81b3 
…hes`
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Nov 11, 2024
Merged
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Nov 11, 2024
robot-clickhouse added a commit that referenced this pull request Nov 11, 2024
@robot-clickhouse
Merge pull request #71762 from ClickHouse/backport/24.9/71071
1a5738d 
Backport #71071 to 24.9: Remove bad test `test_system_replicated_fetches`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Algunenano Algunenano Algunenano approved these changes

Assignees

@Algunenano Algunenano

Labels

🍃 green ci 🌿 Fixing flaky tests in CI pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test_system_replicated_fetches is flaky (suppressed)

8 participants

@alexey-milovidov @robot-ch-test-poll4 @Algunenano @robot-clickhouse @robot-ch-test-poll @robot-ch-test-poll2 @robot-ch-test-poll3 @robot-clickhouse-ci-2