kamcheungting-db commented on code in PR #633:
URL: https://github.com/apache/iceberg-cpp/pull/633#discussion_r3179249676
##########
src/iceberg/util/retry_util.h:
##########
@@ -121,89 +162,34 @@ class ICEBERG_EXPORT RetryRunner {
}
private:
+ enum class RetryPolicyMode {
+ kUnset,
+ kOnlyRetryOn,
+ kStopRetryOn,
+ };
Review Comment:
could you add one line comments for these RetryPolicyModes' behavior?
##########
src/iceberg/util/retry_util.h:
##########
Review Comment:
should we record the result detail for each failed run?
##########
src/iceberg/util/retry_util_internal.h:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <chrono>
+#include <cstdint>
+#include <functional>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+struct RetryTestHooks {
+ using Clock = std::chrono::steady_clock;
+ using Duration = std::chrono::milliseconds;
+ using TimePoint = Clock::time_point;
Review Comment:
these are duplicated with these [few line
code](https://github.com/apache/iceberg-cpp/pull/633/changes#diff-e67d5eeb935f4299db9ad0d2c4115fff174f89f73c4f41d740335338293c3d7bL124-L126)
Have we considered about deduplicating them?
##########
src/iceberg/util/retry_util.cc:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/retry_util.h"
+
+#include <algorithm>
+#include <chrono>
+#include <cmath>
+#include <cstdint>
+#include <limits>
+#include <optional>
+#include <random>
+#include <thread>
+
+#include "iceberg/util/retry_util_internal.h"
+
+namespace iceberg {
+namespace {
+
+const RetryTestHooks*& ActiveRetryTestHooks() {
+ // Keep test hooks thread-local so fake retry timing in one test thread does
not
+ // leak into unrelated retry work or require synchronization around a global
pointer.
+ static thread_local const RetryTestHooks* active_retry_test_hooks = nullptr;
+ return active_retry_test_hooks;
+}
+
+RetryTestHooks::TimePoint RetryNow() {
+ const auto* hooks = GetActiveRetryTestHooks();
+ if (hooks != nullptr && hooks->now) {
+ return hooks->now();
+ }
+ return RetryTestHooks::Clock::now();
+}
+
+void RetrySleepFor(RetryTestHooks::Duration duration) {
+ const auto* hooks = GetActiveRetryTestHooks();
+ if (hooks != nullptr && hooks->sleep_for) {
+ hooks->sleep_for(duration);
+ return;
+ }
+ std::this_thread::sleep_for(duration);
+}
+
+int32_t ApplyRetryJitter(int32_t base_delay_ms) {
+ const auto* hooks = GetActiveRetryTestHooks();
+ if (hooks != nullptr && hooks->jitter) {
+ return hooks->jitter(base_delay_ms);
+ }
+
+ static thread_local std::mt19937 gen(std::random_device{}());
+ const int32_t jitter_range = std::max(1, base_delay_ms / 10);
+ std::uniform_int_distribution<> dis(0, jitter_range - 1);
+ const int64_t jittered_delay_ms = static_cast<int64_t>(base_delay_ms) +
dis(gen);
+ return static_cast<int32_t>(
+ std::min<int64_t>(jittered_delay_ms,
std::numeric_limits<int32_t>::max()));
+}
+
+} // namespace
+
+const RetryTestHooks* GetActiveRetryTestHooks() { return
ActiveRetryTestHooks(); }
+
+void SetActiveRetryTestHooks(const RetryTestHooks* hooks) {
+ ActiveRetryTestHooks() = hooks;
+}
+
+Status RetryRunner::ValidateConfig() const {
+ if (config_.num_retries < 0) {
+ return InvalidArgument("num_retries must be non-negative, got {}",
+ config_.num_retries);
+ }
+ if (config_.num_retries == 0) {
+ return {};
Review Comment:
nitpick: could we explicitly call out?
```suggestion
return RetryExhausted(<".... list of failed run reason">);
```
##########
src/iceberg/util/retry_util.h:
##########
@@ -121,89 +162,34 @@ class ICEBERG_EXPORT RetryRunner {
}
private:
+ enum class RetryPolicyMode {
+ kUnset,
+ kOnlyRetryOn,
+ kStopRetryOn,
+ };
+
using Clock = std::chrono::steady_clock;
using Duration = std::chrono::milliseconds;
using TimePoint = Clock::time_point;
- std::optional<TimePoint> ComputeDeadline() const {
- if (config_.total_timeout_ms <= 0) {
- return std::nullopt;
- }
- return Clock::now() + Duration(config_.total_timeout_ms);
- }
-
- bool HasTimedOut(const std::optional<TimePoint>& deadline) const {
- return deadline.has_value() && Clock::now() >= *deadline;
- }
+ Status ValidateConfig() const;
Review Comment:
please add simple comments for this method. Its implementation is not trivial
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
