Skip to content

Disable the setting s3_slow_all_threads_after_retryable_error by default#87198

Merged
nikitamikhaylov merged 4 commits intomasterfrom
disable-backoff-by-default
Sep 22, 2025
Merged

Disable the setting s3_slow_all_threads_after_retryable_error by default#87198
nikitamikhaylov merged 4 commits intomasterfrom
disable-backoff-by-default

Conversation

@nikitamikhaylov
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Disable s3_slow_all_threads_after_retryable_error by default.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Sep 16, 2025

Workflow [PR], commit [5d3adac]

Summary:

job_name test_name status info comment
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel) failure
02167_format_from_file_extension FAIL
02931_size_virtual_column_use_structure_from_insertion_table FAIL
Exception in test runner FAIL
Killed by signal (in clickhouse-server.log or clickhouse-server.err.log) FAIL
Fatal messages (in clickhouse-server.log or clickhouse-server.err.log) FAIL
Stress test (amd_tsan) failure
Server died FAIL
Hung check failed, possible deadlock found (see hung_check.log) FAIL
Killed by signal (in clickhouse-server.log) FAIL
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL
Killed by signal (output files) FAIL
Found signal in gdb.log FAIL
Stress test (arm_asan) failure
Server died FAIL
Hung check failed, possible deadlock found (see hung_check.log) FAIL
Killed by signal (in clickhouse-server.log) FAIL
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL
Killed by signal (output files) FAIL

@clickhouse-gh clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Sep 16, 2025
@SmitaRKulkarni SmitaRKulkarni self-assigned this Sep 19, 2025
@nikitamikhaylov nikitamikhaylov force-pushed the disable-backoff-by-default branch 3 times, most recently from 0d5bd14 to 8b225c7 Compare September 22, 2025 10:46
@nikitamikhaylov nikitamikhaylov force-pushed the disable-backoff-by-default branch from a3daf50 to 5d3adac Compare September 22, 2025 14:34
@nikitamikhaylov nikitamikhaylov added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Sep 22, 2025
@nikitamikhaylov
Copy link
Copy Markdown
Member Author

All test failures are Invalid number of rows in Chunk.

@nikitamikhaylov nikitamikhaylov added this pull request to the merge queue Sep 22, 2025
Merged via the queue into master with commit d0facf0 Sep 22, 2025
119 of 123 checks passed
@nikitamikhaylov nikitamikhaylov deleted the disable-backoff-by-default branch September 22, 2025 18:57
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 22, 2025
@robot-ch-test-poll2 robot-ch-test-poll2 added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Sep 22, 2025
robot-ch-test-poll1 added a commit that referenced this pull request Sep 22, 2025
Cherry pick #87198 to 25.6: Disable the setting by default
robot-ch-test-poll1 added a commit that referenced this pull request Sep 22, 2025
Cherry pick #87198 to 25.8: Disable the setting by default
robot-ch-test-poll1 added a commit that referenced this pull request Sep 22, 2025
Cherry pick #87198 to 25.9: Disable the setting by default
@nikitamikhaylov nikitamikhaylov changed the title Disable the setting by default Disable the setting s3_slow_all_threads_after_retryable_error by default Sep 22, 2025
nikitamikhaylov added a commit that referenced this pull request Sep 22, 2025
Backport #87198 to 25.6: Disable the setting by default
nikitamikhaylov added a commit that referenced this pull request Sep 22, 2025
Backport #87198 to 25.8: Disable the setting by default
nikitamikhaylov added a commit that referenced this pull request Sep 22, 2025
Backport #87198 to 25.9: Disable the setting by default
@robot-clickhouse robot-clickhouse added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 22, 2025
@azat
Copy link
Copy Markdown
Member

azat commented Sep 23, 2025

You forgot to update the reference file for tests - https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=87439&sha=009d5e9107880bc49acd79582f01cc6c6e226fb0&name_0=BackportPR&name_1=Stateless%20tests%20%28amd_asan%2C%20distributed%20plan%2C%20parallel%2C%202%2F2%29

The problem is that this PR has been created before release, and so CI did not show this

While backports has been merged w/o waiting for CI AFAICS which will show this problem

Should be fixed by #87449

azat added a commit that referenced this pull request Sep 23, 2025
…25.9/87449

* u/backport/25.9/87449:
  Backport #87426 to 25.9: Ignore only not found errors for s3_plain_rewritable (and some other S3 code)
  Backport #87231 to 25.9: Fix "Too large size passed to allocator" UB in JOIN due to mixed const and non-const blocks
  Backport #87198 to 25.9: Disable the setting by default
  Backport #87392 to 25.9: Fix EmbeddedRocksDB upgrade
  Backport #87140 to 25.9: fix: fix server level max temporary size limit
  Backport #87178 to 25.9: PR: fix LEFT/INNER ... RIGHT ... JOINS chain
  Update autogenerated version to 25.10.1.1 and contributors
{"use_skip_indexes_on_data_read", false, false, "New setting"},
{"query_condition_cache_selectivity_threshold", 1.0, 1.0, "New setting."},
{"s3_slow_all_threads_after_retryable_error", true, true, "Added an alias for setting `backup_slow_all_threads_after_retryable_s3_error`"},
{"s3_slow_all_threads_after_retryable_error", false, false, "Added an alias for setting `backup_slow_all_threads_after_retryable_s3_error`"},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it correct that {"s3_slow_all_threads_after_retryable_error", false, false, [...] occurs 2x in the 25.10 section and the 25.9 section?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, also in 25.8 and 25.6

Copy link
Copy Markdown
Member

@rschu1ze rschu1ze Sep 24, 2025

Choose a reason for hiding this comment

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

I think this was done because of backports.

For master, it will be cleaner to have each entry settings change just once in the version that first introduced it, here: 25.6.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately this is not the case and we need to do so not to break the upgrade check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine with me of course, but it seems surprising.

There is a comment at the beginning of the file:

/// Note: please check if the key already exists to prevent duplicate entries.

The next guy reading this file is tempted to clean up duplicate entries.

@Algunenano Do you have thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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-improvement Pull request with some product improvements pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants