You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "plusplusjiajia (via GitHub)" <gi...@apache.org> on 2026/01/03 16:12:23 UTC
[PR] rest: rebuild resource paths from final config [iceberg-cpp]
plusplusjiajia opened a new pull request, #482:
URL: https://github.com/apache/iceberg-cpp/pull/482
Fix MergeConfigs argument order (defaults, client, overrides)
Recreate ResourcePaths using final URI + prefix after fetching server config: ref:https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L233
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2661875679
##########
src/iceberg/catalog/rest/resource_paths.h:
##########
@@ -44,11 +43,8 @@ class ICEBERG_REST_EXPORT ResourcePaths {
static Result<std::unique_ptr<ResourcePaths>> Make(std::string base_uri,
const std::string& prefix);
- /// \brief Set the base URI of the REST catalog server.
- Status SetBaseUri(const std::string& base_uri);
-
/// \brief Get the /v1/config endpoint path.
- Result<std::string> Config() const;
+ static Result<std::string> Config(const std::string& base_uri);
Review Comment:
It sounds good to remove `SetBaseUri`. However, it leads to confusion to make this a static function. I'd suggest to revert this function as you have created a new ResourcePaths based on the server config.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#issuecomment-3710488693
> I think it makes sense to build resource paths from the final config, and I agree to create a new `ResourcePaths` instance. In this case, the `SetBaseUri` will no longer be necessary. Could you help remove it? Btw, I don't think it's worth changing to the signature of the `ResourcePaths::Config`.
SetBaseUri removed. Do you mean build a ResourcePaths to get config?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "HeartLinked (via GitHub)" <gi...@apache.org>.
HeartLinked commented on PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#issuecomment-3710507887
> > I think it makes sense to build resource paths from the final config, and I agree to create a new `ResourcePaths` instance. In this case, the `SetBaseUri` will no longer be necessary. Could you help remove it? Btw, I don't think it's worth changing to the signature of the `ResourcePaths::Config`.
>
> SetBaseUri removed. Do you mean build a ResourcePaths to get config?
> > I think it makes sense to build resource paths from the final config, and I agree to create a new `ResourcePaths` instance. In this case, the `SetBaseUri` will no longer be necessary. Could you help remove it? Btw, I don't think it's worth changing to the signature of the `ResourcePaths::Config`.
>
> SetBaseUri removed. Do you mean build a ResourcePaths to get config?
Well, sorry—I might not have put it clearly. I just wanted to say that I approve of your changes to src/iceberg/catalog/rest/rest_catalog.cc. :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia closed pull request #482: rest: rebuild resource paths from final config
URL: https://github.com/apache/iceberg-cpp/pull/482
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2661880429
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -132,7 +132,11 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
// Update resource paths based on the final config
ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config->Uri());
- ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri))));
+ ICEBERG_ASSIGN_OR_RAISE(
Review Comment:
I think it is cheap to create a new ResourcePaths. I'm fine with this change and please revert changes from 113 to 115 above.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2662055851
##########
src/iceberg/catalog/rest/resource_paths.h:
##########
@@ -44,11 +43,8 @@ class ICEBERG_REST_EXPORT ResourcePaths {
static Result<std::unique_ptr<ResourcePaths>> Make(std::string base_uri,
const std::string& prefix);
- /// \brief Set the base URI of the REST catalog server.
- Status SetBaseUri(const std::string& base_uri);
-
/// \brief Get the /v1/config endpoint path.
- Result<std::string> Config() const;
+ static Result<std::string> Config(const std::string& base_uri);
Review Comment:
> It sounds good to remove `SetBaseUri`. However, it leads to confusion to make this a static function. I'd suggest to revert this function as you have created a new ResourcePaths based on the server config.
it's reasonable, i've reverted.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2660812503
##########
src/iceberg/catalog/rest/resource_paths.cc:
##########
@@ -46,8 +46,8 @@ Status ResourcePaths::SetBaseUri(const std::string& base_uri) {
return {};
}
-Result<std::string> ResourcePaths::Config() const {
- return std::format("{}/v1/config", base_uri_);
+Result<std::string> ResourcePaths::Config(const std::string& base_uri) {
Review Comment:
Please rebase this PR since this has been committed already.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2659275400
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -132,7 +132,11 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
// Update resource paths based on the final config
ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config->Uri());
- ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri))));
+ ICEBERG_ASSIGN_OR_RAISE(
Review Comment:
I'm not sure if it is worth changing this. In theory the `prefix` should not change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia closed pull request #482: rest: rebuild resource paths from final config
URL: https://github.com/apache/iceberg-cpp/pull/482
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2659508947
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -132,7 +132,11 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
// Update resource paths based on the final config
ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config->Uri());
- ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri))));
+ ICEBERG_ASSIGN_OR_RAISE(
Review Comment:
> I'm not sure if it is worth changing this. In theory the `prefix` should not change.
maybe, we can build after getting config?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2660812503
##########
src/iceberg/catalog/rest/resource_paths.cc:
##########
@@ -46,8 +46,8 @@ Status ResourcePaths::SetBaseUri(const std::string& base_uri) {
return {};
}
-Result<std::string> ResourcePaths::Config() const {
- return std::format("{}/v1/config", base_uri_);
+Result<std::string> ResourcePaths::Config(const std::string& base_uri) {
Review Comment:
Please rebase this PR since this has been committed already.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: build resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2661875679
##########
src/iceberg/catalog/rest/resource_paths.h:
##########
@@ -44,11 +43,8 @@ class ICEBERG_REST_EXPORT ResourcePaths {
static Result<std::unique_ptr<ResourcePaths>> Make(std::string base_uri,
const std::string& prefix);
- /// \brief Set the base URI of the REST catalog server.
- Status SetBaseUri(const std::string& base_uri);
-
/// \brief Get the /v1/config endpoint path.
- Result<std::string> Config() const;
+ static Result<std::string> Config(const std::string& base_uri);
Review Comment:
It sounds good to remove `SetBaseUri`. However, it leads to confusion to make this a static function. I'd suggest revert this function as you have created a new ResourcePaths based on the server config.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2659274887
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -117,7 +117,7 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, config));
std::unique_ptr<RestCatalogProperties> final_config = RestCatalogProperties::FromMap(
- MergeConfigs(server_config.overrides, config.configs(), server_config.defaults));
+ MergeConfigs(server_config.defaults, config.configs(), server_config.overrides));
Review Comment:
Thanks for fixing this! Let's not complicate the PR. I think this change can be added without the test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] rest: rebuild resource paths from final config [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #482:
URL: https://github.com/apache/iceberg-cpp/pull/482#discussion_r2659433714
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -117,7 +117,7 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, config));
std::unique_ptr<RestCatalogProperties> final_config = RestCatalogProperties::FromMap(
- MergeConfigs(server_config.overrides, config.configs(), server_config.defaults));
+ MergeConfigs(server_config.defaults, config.configs(), server_config.overrides));
Review Comment:
> Thanks for fixing this! Let's not complicate the PR. I think this change can be added without the test.
Yes, i create a new pull request for it.https://github.com/apache/iceberg-cpp/pull/483/files
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org