JingsongLi commented on code in PR #274:
URL: https://github.com/apache/paimon-cpp/pull/274#discussion_r3974583504
##########
src/paimon/rest/rest_messages.cpp:
##########
@@ -353,6 +355,22 @@ void GetTableResponse::FromJson(const rapidjson::Value&
obj) noexcept(false) {
audit_.ParseFrom(obj);
}
+rapidjson::Value
GetTableTokenResponse::ToJson(rapidjson::Document::AllocatorType* allocator)
const
+ noexcept(false) {
+ rapidjson::Value obj(rapidjson::kObjectType);
+ obj.AddMember(rapidjson::StringRef(kFieldToken),
+ RapidJsonUtil::SerializeValue(token_, allocator).Move(),
*allocator);
+ obj.AddMember(rapidjson::StringRef(kFieldExpiresAtMillis),
+ RapidJsonUtil::SerializeValue(expires_at_millis_,
allocator).Move(), *allocator);
+ return obj;
+}
+
+void GetTableTokenResponse::FromJson(const rapidjson::Value& obj)
noexcept(false) {
+ token_ = RapidJsonUtil::DeserializeKeyValue<std::map<std::string,
std::string>>(
+ obj, kFieldToken, {});
Review Comment:
**[P2] Reject a missing or null token instead of falling back to catalog
credentials**
This default makes a missing or `null` `token` indistinguishable from an
explicitly empty object. With existing catalog storage credentials, a
successful HTTP response that omits the token can therefore continue into
delegate construction using those credentials. Java retains a null response
field and fails when
[mergeTokenWithCatalogOptions()](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-common/src/main/java/org/apache/paimon/rest/RESTTokenFileIO.java#L281-L293)
copies it, so it does not turn this malformed response into fallback data
access. Please require a non-null token object and test missing/null separately
from an explicitly empty `{}`.
##########
src/paimon/rest/rest_catalog.cpp:
##########
@@ -414,6 +432,23 @@ std::shared_ptr<FileSystem> RestCatalog::GetFileSystem()
const {
return fs_;
}
+Result<std::shared_ptr<FileSystem>> RestCatalog::GetTableFileSystem(
+ const Identifier& identifier) const {
+ if (!data_token_enabled_) {
+ return fs_;
+ }
+ // The credentials are issued for the data table, so a system table shares
those of
+ // the table it belongs to.
+ PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier,
ToLoadIdentifier(identifier));
+ return table_fs_cache_->Get(
+ load_identifier.GetFullName(),
+ [this, &load_identifier](const std::string&) ->
Result<std::shared_ptr<FileSystem>> {
+ std::shared_ptr<FileSystem> fs =
std::make_shared<RestTokenFileSystem>(
+ api_, api_->GetMergedOptions(), load_identifier);
Review Comment:
**[P2] Preserve Java's token-keyed FileIO cache semantics**
Java's
[RESTTokenFileIO](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-common/src/main/java/org/apache/paimon/rest/RESTTokenFileIO.java#L71-L82)
caches backend FileIO instances by credential map and expiration, with idle
expiration. This instead caches entire table-bound wrappers by their printable
name. For example, `Identifier("a.b", "c")` and `Identifier("a", "b.c")` both
use `a.b.c` and receive the same wrapper, which remains bound to the first
table's token endpoint. Also, after dropping and recreating a table at a
different storage location, a new lookup still receives the old wrapper and its
unexpired credentials because DDL never invalidates this cache. Please preserve
token-keyed backend reuse; if wrappers are additionally cached, use the
complete Identifier and invalidate them on table changes, as Java's
[CachingCatalog](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-core/src/main/java/org/apa
che/paimon/catalog/CachingCatalog.java) does.
##########
src/paimon/rest/rest_token_file_system.cpp:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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 "paimon/rest/rest_token_file_system.h"
+
+#include <mutex>
+#include <utility>
+
+#include "paimon/catalog_options.h"
+#include "paimon/core/core_options.h"
+
+namespace paimon {
+
+namespace {
+/// Declared here rather than taken from the OSS file system, which is an
optional build
+/// component this module must not depend on.
+constexpr const char kOssEndpointOption[] = "fs.oss.endpoint";
+} // namespace
+
+RestTokenFileSystem::RestTokenFileSystem(const std::shared_ptr<RestApi>& api,
+ const std::map<std::string,
std::string>& catalog_options,
+ const Identifier& identifier, Clock
clock)
+ : api_(api),
+ catalog_options_(catalog_options),
+ identifier_(identifier),
+ clock_(std::move(clock)),
+ logger_(Logger::GetLogger("RestTokenFileSystem")) {}
+
+bool RestTokenFileSystem::ShouldRefresh() const {
+ if (!token_) {
+ return true;
+ }
+ int64_t now_millis =
+
std::chrono::duration_cast<std::chrono::milliseconds>(clock_().time_since_epoch()).count();
+ return token_->expires_at_millis - now_millis <
RestApi::kTokenExpirationSafeTimeMillis;
+}
+
+std::map<std::string, std::string> RestTokenFileSystem::MergeTokenOptions(
+ const std::map<std::string, std::string>& token) const {
+ std::map<std::string, std::string> merged = token;
+ // The DLF OSS endpoint overrides the standard one, since the credentials
are issued
+ // for the DLF endpoint rather than for the endpoint the catalog was
configured with.
+ auto dlf_oss_endpoint =
catalog_options_.find(CatalogOptions::DLF_OSS_ENDPOINT);
+ if (dlf_oss_endpoint != catalog_options_.end() &&
!dlf_oss_endpoint->second.empty()) {
+ merged[kOssEndpointOption] = dlf_oss_endpoint->second;
+ }
+ return merged;
+}
+
+Status RestTokenFileSystem::Refresh() const {
+ PAIMON_LOG_INFO(logger_, "begin refresh data token for identifier [%s]",
+ identifier_.ToString().c_str());
+ PAIMON_ASSIGN_OR_RAISE(GetTableTokenResponse response,
api_->LoadTableToken(identifier_));
+ PAIMON_LOG_INFO(logger_, "end refresh data token for identifier [%s]
expiresAtMillis [%ld]",
+ identifier_.ToString().c_str(),
+ static_cast<int64_t>(response.GetExpiresAtMillis()));
+
+ RestToken token{MergeTokenOptions(response.GetToken()),
response.GetExpiresAtMillis()};
+ // The credentials are the only file system options that change, so the
file system is
+ // rebuilt from the catalog options with the credentials merged over them.
+ std::map<std::string, std::string> fs_options = catalog_options_;
+ for (const auto& [key, value] : token.token) {
+ fs_options[key] = value;
+ }
+ PAIMON_ASSIGN_OR_RAISE(CoreOptions core_options,
+ CoreOptions::FromMap(fs_options,
/*specified_file_system=*/nullptr));
Review Comment:
**[P2] Keep ValidToken independent of delegate initialization**
Java's
[validToken()](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-common/src/main/java/org/apache/paimon/rest/RESTTokenFileIO.java#L296-L303)
only refreshes and returns credentials, specifically so native engines can use
their own file system. Here `ValidToken()` calls `Refresh()`, which runs the
full `CoreOptions::FromMap()` before saving the token. That also initializes
data/manifest format factories, so a native integration that only needs
credentials and omits Avro can fail on the default `manifest.format=avro` even
when the REST response is valid. Please separate token refresh from delegate
construction, and let `ValidToken()` perform only the former. A token-only test
without registered format factories would cover this distinction.
--
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]
