Slow down all backup threads after S3 retryable errors, including ‘SlowDown’#84854
Conversation
| auto outcome = request_fn_(request_); | ||
|
|
||
| if (!outcome.IsSuccess() | ||
| /// AWS SDK's built-in per-thread retry logic is disabled. | ||
| && client_configuration.s3_slow_all_threads_after_retryable_error | ||
| && attempt_no + 1 < max_attempts | ||
| /// Retry attempts are managed by the outer loop, so the attemptedRetries argument can be ignored. | ||
| && client_configuration.retryStrategy->ShouldRetry(outcome.GetError(), /*attemptedRetries*/ -1)) | ||
| { | ||
| updateNextTimeToRetryAfterRetryableError(outcome.GetError(), attempt_no); | ||
| continue; | ||
| } | ||
| return outcome; |
There was a problem hiding this comment.
This is the essential part.
| LOG_WARNING(log, "Request failed, now waiting {} ms before attempting again", sleep_ms); | ||
| sleepForMilliseconds(sleep_ms); |
There was a problem hiding this comment.
I think this is incorrect because it changes the behavior of the old versions with s3_slow_all_threads_after_network_error disabled.
src/Core/SettingsChangesHistory.cpp
Outdated
| {"delta_lake_enable_expression_visitor_logging", false, false, "New setting"}, | ||
| {"write_full_path_in_iceberg_metadata", false, false, "New setting."}, | ||
| {"output_format_orc_compression_block_size", 65536, 262144, "New setting"}, | ||
| {"backup_slow_all_threads_after_retryable_s3_error", false, true, "New setting"}, |
There was a problem hiding this comment.
I think it would be better to say is was always so. So, true, true IMO
| @@ -463,9 +463,9 @@ When set to `true` than for all azure requests first two attempts are made with | |||
| When set to `false` than all attempts are made with identical timeouts. | |||
| )", 0) \ | |||
| DECLARE(Bool, s3_slow_all_threads_after_network_error, true, R"( | |||
There was a problem hiding this comment.
Do we really need two separate settings? Couldn't you just modify the logic of s3_slow_all_threads_after_network_error instead of adding a new setting?
There was a problem hiding this comment.
I enabled the new setting only for backups. In other places, wherever the S3 client is configured, the value of this setting is false, unlike s3_slow_all_threads_after_network_error, which is true.
For example:
src/Coordination/KeeperSnapshotManagerS3.cpp
static constexpr bool s3_slow_all_threads_after_network_error = true;
static constexpr bool s3_slow_all_threads_after_retryable_error = false;
The reason for this is that I prefer to avoid any unexpected delays outside of the backup ecosystem, and the setting s3_slow_all_threads_after_network_error is too broad for the use case.
There was a problem hiding this comment.
The setting s3_slow_all_threads_after_network_error was added only to resolve the backup issue, despite the fact it doesn't have the backup word in its name. Since you've found a better solution let's remove the previous setting or at least make it obsolete and make these settings work in exactly the same way. Just to avoid creating unnecessary complexity.
There was a problem hiding this comment.
The setting s3_slow_all_threads_after_network_error was added only to resolve the backup issue, despite the fact it doesn't have the backup word in its name.
It applies not solely to backups:
/home/ubuntu/src/jkartseva/ClickHouse//src/Coordination/KeeperSnapshotManagerS3.cpp: static constexpr bool s3_slow_all_threads_after_network_error = true;
/home/ubuntu/src/jkartseva/ClickHouse//src/Coordination/KeeperSnapshotManagerS3.cpp: s3_slow_all_threads_after_network_error,
/home/ubuntu/src/jkartseva/ClickHouse//src/Databases/DataLake/GlueCatalog.cpp: extern const SettingsBool s3_slow_all_threads_after_network_error;
/home/ubuntu/src/jkartseva/ClickHouse//src/Databases/DataLake/GlueCatalog.cpp: bool s3_slow_all_threads_after_network_error = global_settings[DB::Setting::s3_slow_all_threads_after_network_error];
/home/ubuntu/src/jkartseva/ClickHouse//src/Databases/DataLake/GlueCatalog.cpp: s3_slow_all_threads_after_network_error,
/home/ubuntu/src/jkartseva/ClickHouse//src/Disks/ObjectStorages/S3/diskSettings.cpp: extern const SettingsBool s3_slow_all_threads_after_network_error;
/home/ubuntu/src/jkartseva/ClickHouse//src/Disks/ObjectStorages/S3/diskSettings.cpp: bool s3_slow_all_threads_after_network_error = static_cast<int>(global_settings[Setting::s3_slow_all_threads_after_network_error]);
/home/ubuntu/src/jkartseva/ClickHouse//src/Disks/ObjectStorages/S3/diskSettings.cpp: if (!for_disk_s3 && local_settings.isChanged("s3_slow_all_threads_after_network_error"))
/home/ubuntu/src/jkartseva/ClickHouse//src/Disks/ObjectStorages/S3/diskSettings.cpp: s3_slow_all_threads_after_network_error = static_cast<int>(local_settings[Setting::s3_slow_all_threads_after_network_error]);
/home/ubuntu/src/jkartseva/ClickHouse//src/Disks/ObjectStorages/S3/diskSettings.cpp: s3_slow_all_threads_after_network_error,
This is why I introduced a new backup-specific setting.
I am fine with deprecating s3_slow_all_threads_after_network_error in favor of the new setting, but IMO its value for non-backup cases should be set to false.
There was a problem hiding this comment.
It applies not solely to backups
Yes, but I didn't have any intention to do anything specially for KeeperSnapshotManagerS3 or S3 object storage. My idea in #80035 was to provide a general fix for S3 client. Since you've modified that fix so it works only for backups now, then it seems better not to apply the previous insufficient fix #80035 to KeeperSnapshotManagerS3 or S3 object storage.
|
|
||
|
|
||
| def check_backup_and_restore( | ||
| cluster, |
There was a problem hiding this comment.
Why? It was handy not to always pass argument cluster around.
There was a problem hiding this comment.
I encountered errors when attempting to use the broken_s3 fixture with cluster defined as a global variable.
No idea why — I’ll share what it was.
There was a problem hiding this comment.
Okay, this run successfully: https://pastila.nl/?0007ca17/cd50b03b85732812330d9e5f7d259acc#sV+W48OJYF+VvST8Na3hSA==
So the argument change can be reverted
957e0df
|
Test |
Cherry pick #84854 to 25.6: Slow down all backup threads after S3 retryable errors, including ‘SlowDown’
…ble errors, including ‘SlowDown’
Backport #84854 to 25.6: Slow down all backup threads after S3 retryable errors, including ‘SlowDown’
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Follow-up to #84854
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
Fix rebase issue: - 20250806 ClickHouse#84854 - 20250729 ClickHouse#84011 - 20250318 ClickHouse#77257 (cherry picked from commit e7061da)
backup_slow_all_threads_after_retryable_s3_errorsetting.backup_slow_all_threads_after_retryable_s3_erroris set. This allows for coordinated global backoff, causing all threads to sleep after a retryable S3 error occurs.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Introduce a new
backup_slow_all_threads_after_retryable_s3_errorsetting to reduce pressure on S3 during retry storms caused by errors such asSlowDown, by slowing down all threads once a single retryable error is observed.Documentation entry for user-facing changes