Skip to content

rest: build resource paths from final config #482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
wgtmac merged 3 commits into from
Jan 6, 2026
Merged

rest: build resource paths from final config #482

wgtmac merged 3 commits into from
Jan 6, 2026

Conversation

@plusplusjiajia
Copy link
Member

@plusplusjiajia plusplusjiajia commented Jan 3, 2026 •
edited
Loading

Build 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

@plusplusjiajia plusplusjiajia force-pushed the fix-prefix branch 2 times, most recently from 21775d9 to f2213de Compare January 3, 2026 16:57
@plusplusjiajia plusplusjiajia reopened this Jan 3, 2026
@plusplusjiajia plusplusjiajia reopened this Jan 4, 2026
wgtmac
wgtmac reviewed Jan 4, 2026
View reviewed changes
src/iceberg/catalog/rest/rest_catalog.cc

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));
Copy link
Member

@wgtmac wgtmac Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Let's not complicate the PR. I think this change can be added without the test.

Copy link
Member Author

@plusplusjiajia plusplusjiajia Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

wgtmac
wgtmac reviewed Jan 4, 2026
View reviewed changes
@plusplusjiajia plusplusjiajia force-pushed the fix-prefix branch 2 times, most recently from f2213de to d74a7dc Compare January 4, 2026 06:59
@plusplusjiajia plusplusjiajia changed the title rest: rebuild resource paths from final config rest: build resource paths from final config Jan 4, 2026
HeartLinked
HeartLinked reviewed Jan 5, 2026
View reviewed changes
Copy link
Contributor

@HeartLinked HeartLinked left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to build resource paths from the final config. 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.

@plusplusjiajia
Copy link
Member Author

plusplusjiajia commented Jan 5, 2026

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?

@HeartLinked
Copy link
Contributor

HeartLinked commented Jan 5, 2026 •
edited
Loading

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 agree with your changes to src/iceberg/catalog/rest/rest_catalog.cc. :)

wgtmac
wgtmac reviewed Jan 5, 2026
View reviewed changes
src/iceberg/catalog/rest/resource_paths.h Outdated

/// \brief Get the /v1/config endpoint path.
Result<std::string> Config() const;
static Result<std::string> Config(const std::string& base_uri);
Copy link
Member

@wgtmac wgtmac Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@plusplusjiajia plusplusjiajia Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wgtmac
wgtmac approved these changes Jan 6, 2026
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @plusplusjiajia and for the review @HeartLinked

@wgtmac wgtmac merged commit 95ec7a3 into apache:main Jan 6, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

+1 more reviewer

@HeartLinked HeartLinked HeartLinked approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@plusplusjiajia @HeartLinked @wgtmac