This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch codex/oauth-refresh-executor
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/codex/oauth-refresh-executor
by this push:
new 6a497d79 refactor(rest): reorder catalog factory arguments and clarify
task handles
6a497d79 is described below
commit 6a497d7980cebc64a9514989d6b025b2034306e7
Author: Gang Wu <[email protected]>
AuthorDate: Mon Oct 19 23:20:36 2026 +0800
refactor(rest): reorder catalog factory arguments and clarify task handles
---
src/iceberg/catalog/rest/rest_catalog.cc | 4 ++--
src/iceberg/catalog/rest/rest_catalog.h | 10 +++++-----
src/iceberg/test/auth_manager_test.cc | 9 +++++++--
src/iceberg/test/rest_catalog_unit_test.cc | 16 +++++++---------
src/iceberg/util/scheduled_executor.cc | 6 ++++--
src/iceberg/util/scheduled_executor_internal.h | 4 ++--
6 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/src/iceberg/catalog/rest/rest_catalog.cc
b/src/iceberg/catalog/rest/rest_catalog.cc
index 67a87d15..a0f3d41e 100644
--- a/src/iceberg/catalog/rest/rest_catalog.cc
+++ b/src/iceberg/catalog/rest/rest_catalog.cc
@@ -432,8 +432,8 @@ RestCatalog::~RestCatalog() {
}
Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
- const RestCatalogProperties& config, Executor* metrics_executor,
- ClientFactory client_factory, Executor* auth_executor) {
+ const RestCatalogProperties& config, ClientFactory client_factory,
+ Executor* auth_executor, Executor* metrics_executor) {
if (!client_factory) {
client_factory = [](const RestCatalogProperties& properties)
-> Result<std::shared_ptr<HttpClient>> {
diff --git a/src/iceberg/catalog/rest/rest_catalog.h
b/src/iceberg/catalog/rest/rest_catalog.h
index e1b1a7df..b25f627b 100644
--- a/src/iceberg/catalog/rest/rest_catalog.h
+++ b/src/iceberg/catalog/rest/rest_catalog.h
@@ -58,18 +58,18 @@ class ICEBERG_REST_EXPORT RestCatalog final
std::function<Result<std::shared_ptr<HttpClient>>(const
RestCatalogProperties&)>;
/// \brief Create a RestCatalog instance.
- /// \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.
/// \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.
static Result<std::shared_ptr<RestCatalog>> Make(const
RestCatalogProperties& config,
- Executor* metrics_executor
= nullptr,
ClientFactory
client_factory = {},
- Executor* auth_executor =
nullptr);
+ Executor* auth_executor =
nullptr,
+ Executor* metrics_executor
= nullptr);
std::string_view name() const override;
diff --git a/src/iceberg/test/auth_manager_test.cc
b/src/iceberg/test/auth_manager_test.cc
index 2c5796eb..3182ef92 100644
--- a/src/iceberg/test/auth_manager_test.cc
+++ b/src/iceberg/test/auth_manager_test.cc
@@ -728,8 +728,11 @@ TEST(ScheduledExecutorTest, CancelPreventsExecution) {
auto handle = scheduler.Schedule(std::chrono::milliseconds(100),
ExecutorTask([&] { fired.store(true); }));
+ EXPECT_TRUE(handle.IsValid());
// Cancel before it fires
handle.Cancel();
+ EXPECT_FALSE(handle.IsValid());
+ handle.Cancel();
// Wait past the scheduled time
std::this_thread::sleep_for(std::chrono::milliseconds(200));
@@ -754,14 +757,14 @@ TEST(ScheduledExecutorTest, ShutdownWithPendingTasks) {
EXPECT_FALSE(fired.load());
}
-// Verifies that Schedule after shutdown returns invalid handle (0)
+// Verifies that Schedule after shutdown returns an invalid handle
TEST(ScheduledExecutorTest, ScheduleAfterShutdownIsNoop) {
test::ThreadExecutor executor;
::iceberg::internal::ScheduledExecutor scheduler(executor);
scheduler.Shutdown();
auto handle = scheduler.Schedule(std::chrono::milliseconds(10),
ExecutorTask([] {}));
- EXPECT_FALSE(handle.valid());
+ EXPECT_FALSE(handle.IsValid());
}
// Verifies that cancelling an invalid handle does not crash
@@ -770,7 +773,9 @@ TEST(ScheduledExecutorTest, CancelInvalidHandleIsNoop) {
::iceberg::internal::ScheduledExecutor scheduler(executor);
// Should not crash
::iceberg::internal::ScheduledTask handle;
+ EXPECT_FALSE(handle.IsValid());
handle.Cancel();
+ EXPECT_FALSE(handle.IsValid());
scheduler.Shutdown();
}
diff --git a/src/iceberg/test/rest_catalog_unit_test.cc
b/src/iceberg/test/rest_catalog_unit_test.cc
index 97c9c365..87ec4b78 100644
--- a/src/iceberg/test/rest_catalog_unit_test.cc
+++ b/src/iceberg/test/rest_catalog_unit_test.cc
@@ -206,7 +206,7 @@ class RestCatalogLoadTableTest : public ::testing::Test {
EXPECT_CALL(*mock_client_, Get(::testing::EndsWith("/v1/config"), _, _, _,
_))
.WillOnce(Return(HttpResponse(200, ToJson(server_config).dump())));
auto config = RestCatalogProperties::FromMap(std::move(props));
- return RestCatalog::Make(config, /*metrics_executor=*/nullptr,
+ return RestCatalog::Make(config,
[client = mock_client_](const auto&) { return
client; });
}
@@ -299,8 +299,7 @@ TEST_F(RestCatalogLoadTableTest,
InitializationPropagatesClientFactoryErrors) {
}
int calls = 0;
auto result = RestCatalog::Make(
- config, /*metrics_executor=*/nullptr,
- [&](const RestCatalogProperties&) ->
Result<std::shared_ptr<HttpClient>> {
+ config, [&](const RestCatalogProperties&) ->
Result<std::shared_ptr<HttpClient>> {
if (++calls == fail_on_call) {
return IOError("HTTP client setup failed");
}
@@ -343,12 +342,11 @@ TEST_F(RestCatalogLoadTableTest,
InitializationUsesMergedServerConfig) {
{"scan-planning-mode", "client"},
{"header.X-Shared", "client"}});
std::vector<RestCatalogProperties> client_configs;
- ICEBERG_UNWRAP_OR_FAIL(auto root,
- RestCatalog::Make(config,
/*metrics_executor=*/nullptr,
- [&](const RestCatalogProperties&
properties) {
-
client_configs.push_back(properties);
- return mock_client_;
- }));
+ ICEBERG_UNWRAP_OR_FAIL(
+ auto root, RestCatalog::Make(config, [&](const RestCatalogProperties&
properties) {
+ client_configs.push_back(properties);
+ return mock_client_;
+ }));
ASSERT_EQ(client_configs.size(), 2);
EXPECT_EQ(client_configs[0].ExtractHeaders().at("X-Shared"), "client");
EXPECT_FALSE(client_configs[0].ExtractHeaders().contains("X-Default"));
diff --git a/src/iceberg/util/scheduled_executor.cc
b/src/iceberg/util/scheduled_executor.cc
index 2012f5ba..6d130450 100644
--- a/src/iceberg/util/scheduled_executor.cc
+++ b/src/iceberg/util/scheduled_executor.cc
@@ -24,9 +24,11 @@
namespace iceberg::internal {
void ScheduledTask::Cancel() {
- if (auto state = state_.lock()) state->Cancel(id_);
+ if (id_.has_value()) {
+ if (auto state = state_.lock()) state->Cancel(*id_);
+ }
state_.reset();
- id_ = 0;
+ id_.reset();
}
ScheduledExecutorState::ScheduledExecutorState(Executor& executor)
diff --git a/src/iceberg/util/scheduled_executor_internal.h
b/src/iceberg/util/scheduled_executor_internal.h
index 3975a094..170d4eb1 100644
--- a/src/iceberg/util/scheduled_executor_internal.h
+++ b/src/iceberg/util/scheduled_executor_internal.h
@@ -47,11 +47,11 @@ class ICEBERG_EXPORT ScheduledTask {
: state_(std::move(state)), id_(id) {}
void Cancel();
- bool valid() const { return id_ != 0 && !state_.expired(); }
+ bool IsValid() const { return id_.has_value() && !state_.expired(); }
private:
std::weak_ptr<ScheduledExecutorState> state_;
- uint64_t id_ = 0;
+ std::optional<uint64_t> id_;
};
class ICEBERG_EXPORT ScheduledExecutorState
