Skip to content

Commit 7e14509

Browse files
wgtmacwgtmac
committed
address feedback and fix ci
1 parent dc075f1 commit 7e14509

4 files changed

Lines changed: 68 additions & 45 deletions

File tree

‎src/iceberg/test/retry_util_test.cc‎

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ TEST(RetryRunnerTest, StopRetryOnMatchingError) {
235235
.min_wait_ms = 1,
236236
.max_wait_ms = 10,
237237
.total_timeout_ms = 5000})
238-
.StopRetryOn({ErrorKind::kCommitStateUnknown})
238+
.StopRetryOn(ErrorKind::kCommitStateUnknown)
239239
.Run(
240240
[&]() -> Result<int> {
241241
++call_count;
@@ -322,38 +322,38 @@ TEST(RetryRunnerTest, InvalidBackoffConfigFailsBeforeTaskRuns) {
322322
};
323323

324324
const std::vector<InvalidConfigCase> test_cases = {
325-
{RetryConfig{.num_retries = std::numeric_limits<int32_t>::max(),
326-
.min_wait_ms = 1,
327-
.max_wait_ms = 10,
328-
.total_timeout_ms = 5000},
329-
"num_retries is too large"},
330-
{RetryConfig{.num_retries = 1,
331-
.min_wait_ms = 0,
332-
.max_wait_ms = 10,
333-
.total_timeout_ms = 5000},
334-
"min_wait_ms must be positive"},
335-
{RetryConfig{.num_retries = 1,
336-
.min_wait_ms = 1,
337-
.max_wait_ms = 0,
338-
.total_timeout_ms = 5000},
339-
"max_wait_ms must be positive"},
340-
{RetryConfig{.num_retries = 1,
341-
.min_wait_ms = 20,
342-
.max_wait_ms = 10,
343-
.total_timeout_ms = 5000},
344-
"max_wait_ms must be greater than or equal to min_wait_ms"},
345-
{RetryConfig{.num_retries = 1,
346-
.min_wait_ms = 1,
347-
.max_wait_ms = 10,
348-
.total_timeout_ms = 5000,
349-
.scale_factor = 0.5},
350-
"scale_factor must be finite and at least 1.0"},
351-
{RetryConfig{.num_retries = 1,
352-
.min_wait_ms = 1,
353-
.max_wait_ms = 10,
354-
.total_timeout_ms = 5000,
355-
.scale_factor = std::numeric_limits<double>::infinity()},
356-
"scale_factor must be finite and at least 1.0"},
325+
{.config = RetryConfig{.num_retries = std::numeric_limits<int32_t>::max(),
326+
.min_wait_ms = 1,
327+
.max_wait_ms = 10,
328+
.total_timeout_ms = 5000},
329+
.expected_message = "num_retries is too large"},
330+
{.config = RetryConfig{.num_retries = 1,
331+
.min_wait_ms = 0,
332+
.max_wait_ms = 10,
333+
.total_timeout_ms = 5000},
334+
.expected_message = "min_wait_ms must be positive"},
335+
{.config = RetryConfig{.num_retries = 1,
336+
.min_wait_ms = 1,
337+
.max_wait_ms = 0,
338+
.total_timeout_ms = 5000},
339+
.expected_message = "max_wait_ms must be positive"},
340+
{.config = RetryConfig{.num_retries = 1,
341+
.min_wait_ms = 20,
342+
.max_wait_ms = 10,
343+
.total_timeout_ms = 5000},
344+
.expected_message = "max_wait_ms must be greater than or equal to min_wait_ms"},
345+
{.config = RetryConfig{.num_retries = 1,
346+
.min_wait_ms = 1,
347+
.max_wait_ms = 10,
348+
.total_timeout_ms = 5000,
349+
.scale_factor = 0.5},
350+
.expected_message = "scale_factor must be finite and at least 1.0"},
351+
{.config = RetryConfig{.num_retries = 1,
352+
.min_wait_ms = 1,
353+
.max_wait_ms = 10,
354+
.total_timeout_ms = 5000,
355+
.scale_factor = std::numeric_limits<double>::infinity()},
356+
.expected_message = "scale_factor must be finite and at least 1.0"},
357357
};
358358

359359
for (const auto& test_case : test_cases) {

‎src/iceberg/util/retry_util.cc‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,24 +33,34 @@
3333
namespace iceberg {
3434
namespace {
3535

36+
const RetryTestHooks*& ActiveRetryTestHooks() {
37+
// Keep test hooks thread-local so fake retry timing in one test thread does not
38+
// leak into unrelated retry work or require synchronization around a global pointer.
39+
static thread_local const RetryTestHooks* active_retry_test_hooks = nullptr;
40+
return active_retry_test_hooks;
41+
}
42+
3643
RetryTestHooks::TimePoint RetryNow() {
37-
if (active_retry_test_hooks != nullptr && active_retry_test_hooks->now) {
38-
return active_retry_test_hooks->now();
44+
const auto* hooks = GetActiveRetryTestHooks();
45+
if (hooks != nullptr && hooks->now) {
46+
return hooks->now();
3947
}
4048
return RetryTestHooks::Clock::now();
4149
}
4250

4351
void RetrySleepFor(RetryTestHooks::Duration duration) {
44-
if (active_retry_test_hooks != nullptr && active_retry_test_hooks->sleep_for) {
45-
active_retry_test_hooks->sleep_for(duration);
52+
const auto* hooks = GetActiveRetryTestHooks();
53+
if (hooks != nullptr && hooks->sleep_for) {
54+
hooks->sleep_for(duration);
4655
return;
4756
}
4857
std::this_thread::sleep_for(duration);
4958
}
5059

5160
int32_t ApplyRetryJitter(int32_t base_delay_ms) {
52-
if (active_retry_test_hooks != nullptr && active_retry_test_hooks->jitter) {
53-
return active_retry_test_hooks->jitter(base_delay_ms);
61+
const auto* hooks = GetActiveRetryTestHooks();
62+
if (hooks != nullptr && hooks->jitter) {
63+
return hooks->jitter(base_delay_ms);
5464
}
5565

5666
static thread_local std::mt19937 gen(std::random_device{}());
@@ -63,6 +73,12 @@ int32_t ApplyRetryJitter(int32_t base_delay_ms) {
6373

6474
} // namespace
6575

76+
const RetryTestHooks* GetActiveRetryTestHooks() { return ActiveRetryTestHooks(); }
77+
78+
void SetActiveRetryTestHooks(const RetryTestHooks* hooks) {
79+
ActiveRetryTestHooks() = hooks;
80+
}
81+
6682
Status RetryRunner::ValidateConfig() const {
6783
if (config_.num_retries < 0) {
6884
return InvalidArgument("num_retries must be non-negative, got {}",

‎src/iceberg/util/retry_util.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,12 @@ class ICEBERG_EXPORT RetryRunner {
114114
return *this;
115115
}
116116

117+
/// \brief Specify a single error type that should stop retries immediately.
118+
///
119+
/// \note OnlyRetryOn takes priority over StopRetryOn. If OnlyRetryOn is set,
120+
/// StopRetryOn is ignored.
121+
RetryRunner& StopRetryOn(ErrorKind error_kind) { return StopRetryOn({error_kind}); }
122+
117123
/// \brief Run a task that returns a Result<T>
118124
///
119125
/// When `num_retries > 0`, the retry policy must be configured explicitly via

‎src/iceberg/util/retry_util_internal.h‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#include <cstdint>
2424
#include <functional>
2525

26+
#include "iceberg/iceberg_export.h"
27+
2628
namespace iceberg {
2729

2830
struct RetryTestHooks {
@@ -35,23 +37,22 @@ struct RetryTestHooks {
3537
std::function<int32_t(int32_t)> jitter;
3638
};
3739

38-
// Keep test hooks thread-local so fake retry timing in one test thread does not
39-
// leak into unrelated retry work or require synchronization around a global pointer.
40-
inline thread_local const RetryTestHooks* active_retry_test_hooks = nullptr;
40+
ICEBERG_EXPORT const RetryTestHooks* GetActiveRetryTestHooks();
41+
ICEBERG_EXPORT void SetActiveRetryTestHooks(const RetryTestHooks* hooks);
4142

4243
class ScopedRetryTestHooks {
4344
public:
4445
explicit ScopedRetryTestHooks(const RetryTestHooks& hooks)
45-
: previous_hooks_(active_retry_test_hooks) {
46-
active_retry_test_hooks = &hooks;
46+
: previous_hooks_(GetActiveRetryTestHooks()) {
47+
SetActiveRetryTestHooks(&hooks);
4748
}
4849

4950
ScopedRetryTestHooks(const ScopedRetryTestHooks&) = delete;
5051
ScopedRetryTestHooks& operator=(const ScopedRetryTestHooks&) = delete;
5152
ScopedRetryTestHooks(ScopedRetryTestHooks&&) = delete;
5253
ScopedRetryTestHooks& operator=(ScopedRetryTestHooks&&) = delete;
5354

54-
~ScopedRetryTestHooks() { active_retry_test_hooks = previous_hooks_; }
55+
~ScopedRetryTestHooks() { SetActiveRetryTestHooks(previous_hooks_); }
5556

5657
private:
5758
const RetryTestHooks* previous_hooks_;

0 commit comments

Comments
 (0)