wgtmac commented on code in PR #1033:
URL: https://github.com/apache/iceberg-cpp/pull/1033#discussion_r4320398643
##########
src/iceberg/catalog/rest/rest_catalog.h:
##########
@@ -58,14 +58,18 @@ class ICEBERG_REST_EXPORT RestCatalog final
std::function<Result<std::shared_ptr<HttpClient>>(const
RestCatalogProperties&)>;
/// \brief Create a RestCatalog instance.
+ /// \param client_factory Optional HTTP client factory, called with the
initial config
+ /// and then the merged server config. When empty, uses the default HTTP
client.
+ /// \param auth_executor Optional non-owning executor for OAuth2 token
refresh. When
+ /// null, background token refresh is disabled. When non-null, it must
outlive the
+ /// catalog and all authentication sessions created by it.
/// \param metrics_executor Optional non-owning executor for asynchronous
REST metrics.
/// It must outlive the catalog and all tables created by it. When null,
REST metrics
/// are reported synchronously.
- /// \param client_factory Optional HTTP client factory, called with the
initial config
- /// and then the merged server config. When empty, uses the default HTTP
client.
static Result<std::shared_ptr<RestCatalog>> Make(const
RestCatalogProperties& config,
- Executor* metrics_executor
= nullptr,
- ClientFactory
client_factory = {});
+ ClientFactory
client_factory = {},
+ Executor* auth_executor =
nullptr,
+ Executor* metrics_executor
= nullptr);
std::string_view name() const override;
Review Comment:
The source-level change is intentional. This PR explicitly adopts
Make(config, client_factory, auth_executor, metrics_executor), and all
repository call sites have been updated. We accept the positional API change
rather than retain a second compatibility overload or add an options type.
Resolving this as an accepted API decision, not as a claim that the old
positional signature remains compatible.
##########
src/iceberg/util/scheduled_executor.cc:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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 <utility>
+
+#include "iceberg/util/scheduled_executor_internal.h"
+
+namespace iceberg::internal {
+
+void ScheduledTask::Cancel() {
+ if (id_.has_value()) {
+ if (auto state = state_.lock()) state->Cancel(*id_);
+ }
+ state_.reset();
+ id_.reset();
+}
+
+ScheduledExecutorState::ScheduledExecutorState(Executor& executor)
+ : executor_(executor) {}
+
+ScheduledExecutorState::~ScheduledExecutorState() { Shutdown(); }
+
+void ScheduledExecutorState::Start() {
+ auto self = shared_from_this();
+ timer_thread_ = std::thread([self = std::move(self)] { self->Run(); });
+}
+
+ScheduledTask ScheduledExecutorState::Schedule(
+ std::chrono::steady_clock::time_point deadline, ExecutorTask task,
+ std::optional<ScheduledTaskRejection> on_rejected) {
+ std::lock_guard lock(mutex_);
+ if (shutdown_) return {};
+ auto id = next_id_++;
+ tasks_.push(Task{.id = id,
+ .deadline = deadline,
+ .task = std::move(task),
+ .on_rejected = std::move(on_rejected)});
+ scheduled_.insert(id);
Review Comment:
There is no unused-variable build failure here. The structured-binding
declaration is used through it, which is stored in by_id_; an unused element
does not make the whole declaration unused. The non-trivial optional<Task>
named cancelled is deliberately retained until after the queue lock is
released, so destroying its callback captures can safely re-enter scheduler
code. That lifetime is documented next to the scope. The current head 780956480
passes cpp-linter and the Linux, macOS and Windows build checks; no warning
suppression or behavior change is needed.
##########
src/iceberg/util/scheduled_executor.cc:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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 <utility>
+
+#include "iceberg/util/scheduled_executor_internal.h"
+
+namespace iceberg::internal {
+
+void ScheduledTask::Cancel() {
+ if (id_.has_value()) {
+ if (auto state = state_.lock()) state->Cancel(*id_);
+ }
+ state_.reset();
+ id_.reset();
+}
+
+ScheduledExecutorState::ScheduledExecutorState(Executor& executor)
+ : executor_(executor) {}
+
+ScheduledExecutorState::~ScheduledExecutorState() { Shutdown(); }
+
+void ScheduledExecutorState::Start() {
+ auto self = shared_from_this();
+ timer_thread_ = std::thread([self = std::move(self)] { self->Run(); });
+}
+
+ScheduledTask ScheduledExecutorState::Schedule(
+ std::chrono::steady_clock::time_point deadline, ExecutorTask task,
+ std::optional<ScheduledTaskRejection> on_rejected) {
+ std::lock_guard lock(mutex_);
+ if (shutdown_) return {};
+ auto id = next_id_++;
+ tasks_.push(Task{.id = id,
+ .deadline = deadline,
+ .task = std::move(task),
+ .on_rejected = std::move(on_rejected)});
+ scheduled_.insert(id);
+ cv_.notify_one();
+ return {weak_from_this(), id};
+}
+
+void ScheduledExecutorState::Cancel(uint64_t id) {
+ if (id == 0) return;
+ {
+ std::lock_guard lock(mutex_);
Review Comment:
There is no unused-variable build failure here. The structured-binding
declaration is used through it, which is stored in by_id_; an unused element
does not make the whole declaration unused. The non-trivial optional<Task>
named cancelled is deliberately retained until after the queue lock is
released, so destroying its callback captures can safely re-enter scheduler
code. That lifetime is documented next to the scope. The current head 780956480
passes cpp-linter and the Linux, macOS and Windows build checks; no warning
suppression or behavior change is needed.
##########
src/iceberg/catalog/rest/rest_catalog.h:
##########
@@ -58,14 +58,19 @@ class ICEBERG_REST_EXPORT RestCatalog final
std::function<Result<std::shared_ptr<HttpClient>>(const
RestCatalogProperties&)>;
/// \brief Create a RestCatalog instance.
+ /// \param client_factory Optional HTTP client factory, called with the
initial config
+ /// and then the merged server config. When empty, uses the default HTTP
client.
+ /// \param auth_executor Optional non-owning executor for OAuth2 token
refresh. When
+ /// null, background token refresh is disabled. When non-null, it must
outlive the
+ /// catalog and all authentication sessions created by it. It must enqueue
work
+ /// asynchronously without blocking the shared timer thread.
/// \param metrics_executor Optional non-owning executor for asynchronous
REST metrics.
/// It must outlive the catalog and all tables created by it. When null,
REST metrics
/// are reported synchronously.
- /// \param client_factory Optional HTTP client factory, called with the
initial config
- /// and then the merged server config. When empty, uses the default HTTP
client.
static Result<std::shared_ptr<RestCatalog>> Make(const
RestCatalogProperties& config,
- Executor* metrics_executor
= nullptr,
- ClientFactory
client_factory = {});
+ ClientFactory
client_factory = {},
+ Executor* auth_executor =
nullptr,
+ Executor* metrics_executor
= nullptr);
Review Comment:
The source-level change is intentional. This PR explicitly adopts
Make(config, client_factory, auth_executor, metrics_executor), and all
repository call sites have been updated. We accept the positional API change
rather than retain a second compatibility overload or add an options type.
Resolving this as an accepted API decision, not as a claim that the old
positional signature remains compatible.
--
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]
