Name the S3 disk retry options as the code reads them - #118955
Conversation
In a `storage_configuration` disk section of type `s3`, nothing reads the documented `retry_attempts` and `single_read_retries`: `S3Settings::loadFromConfigForObjectStorage` builds `S3RequestSettings` with the `s3_` prefix, so the keys probed are `s3_retry_attempts` and `s3_max_single_read_retries`. A user who lowers the retry count to fail over to another volume quickly silently keeps the default of 500 retries with growing backoff. The documentation page and the example embedded in the server source now name the options the code reads, and the listed default is corrected - `S3::DEFAULT_RETRY_ATTEMPTS` is 500, not 10. Teaching the code to accept the documented spelling instead would be the wrong half of the fix: a configuration that has silently carried `retry_attempts` for years would suddenly take effect, and `0` is exactly the value some of them carry. The `retry_attempts` spellings in this repository's own integration-test configurations are left alone for the same reason: they are dead keys, and making them live would change what those tests exercise. Only the source of truth is edited; the generated region of the MergeTree documentation page is left to the documentation autogeneration workflow.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
Workflow [PR], commit [4bf00b9] Summary: ❌
AI ReviewSummaryThis PR fixes the two S3 disk retry option names in the English configuration docs and the PR MetadataThe Findings
Final VerdictChanges requested. LLVM Coverage ReportMeasured on commit 4bf00b9.
Changed lines: Changed C/C++ lines covered: 8/8 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-documentation
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| | `request_timeout_ms` | Request timeout in milliseconds. | `5000` (5 seconds) | | ||
| | `retry_attempts` | Number of retry attempts for failed requests. | `10` | | ||
| | `single_read_retries` | Number of retry attempts for connection drops during read. | `4` | | ||
| | `s3_retry_attempts` | Number of retry attempts for failed requests. | `500` | |
There was a problem hiding this comment.
This table is still not fully aligned with the parser: S3Settings::loadFromConfigForObjectStorage overwrites the disk read-only flag only from config_prefix + ".readonly" (src/IO/S3Settings.cpp:51), so the read_only spelling that remains lower in this same table is still a dead option. Since this PR is fixing dead disk config names, can we rename that row to readonly too? Otherwise readers can copy a config that leaves the disk writable.
clickhouse-gh
Bot
added
the
comp-documentation
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units7 translation units recompiled, 16 s compile time in total, 7 of them have a recent master baseline. |
|
The one red check, |
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
3 participants
In a
storage_configurationdisk section of types3, the documented optionsretry_attemptsandsingle_read_retriesare read by nothing:S3Settings::loadFromConfigForObjectStoragebuildsS3RequestSettingswith thes3_prefix, so the keys the code probes ares3_retry_attemptsands3_max_single_read_retries. A user who lowers the retry count to fail over to another volume quickly silently keeps the default of 500 retries with growing backoff.The documentation and the example embedded in the server source now name the options the code reads. Renaming what the code accepts instead would be the wrong half of the fix: a configuration that has silently carried
retry_attemptsfor years would suddenly take effect, andretry_attemptsset to0is exactly the value some of them carry. The listed default is corrected too -S3::DEFAULT_RETRY_ATTEMPTSis 500, not 10.The
retry_attemptsspellings left in this repository's own integration-test configurations are deliberately untouched: they are dead keys, and making them live would change what those tests exercise.Only the source of truth is edited; the generated region of
docs/reference/engines/table-engines/mergetree-family/mergetree.mdxis left to the documentation autogeneration workflow.Closes: #118444
Related: #118440
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
...
Workflow [PR]
Sync PR [sync-upstream/pr/118955]
Version info
26.9.1.1077(included in26.9and later)