-
Notifications
You must be signed in to change notification settings - Fork 86
feat: scaffolding work of rest catalog client #296
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.
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
Conversation
64e53e3 to
674c8ab
Compare
wgtmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did an initial pass, the main concerns are:
- URL encoding & decoding may not match the Java impl.
- Namespace encoding is using the deprecated
0x1F. - ErrorResponse and ErrorModel are duplicate. We can improve it by using a single ErrorResponse.
Missing test cases of:
- Catalog properties to extract header map.
- All functions in the rest_util.h, check
TestRestUtil.java. - Resource paths, check
TestResourcePaths.java.
Since this is a large PR and we don't have good test fixture for it yet, I'm fine to add them later by adding TODOs.
| constexpr std::string_view kHeaderAccept = "Accept"; | ||
| constexpr std::string_view kHeaderXClientVersion = "X-Client-Version"; | ||
| constexpr std::string_view kHeaderUserAgent = "User-Agent"; | ||
| inline const std::string kHeaderContentType = "Content-Type"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you apply this change? I would prefer constexpr std::string_view instead of const std::string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is that we still need to wrap it with std::string every time to create http headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @HeartLinked may need to remove std::string(...) wrapper in some files, such as src/iceberg/catalog/rest/config.cc.
| }; | ||
|
|
||
| TEST_F(RestCatalogTest, MakeCatalogSuccess) { | ||
| TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you disable the test? Leave a comment with a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary integration test that requires a local REST server, such as the Docker image apache/iceberg-rest-fixture provided by Apache Iceberg. Currently, this method allows testing locally, but it obviously cannot pass the Github CI process, so it is a temporary expedient. In this first version PR, we clearly cannot comprehensively complete such a large amount of work. You can take a look on this issue: #333 :)
6fd456f to
1d924f9
Compare
17c0a53 to
1b17be0
Compare
| constexpr std::string_view kHeaderAccept = "Accept"; | ||
| constexpr std::string_view kHeaderXClientVersion = "X-Client-Version"; | ||
| constexpr std::string_view kHeaderUserAgent = "User-Agent"; | ||
| inline const std::string kHeaderContentType = "Content-Type"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is that we still need to wrap it with std::string every time to create http headers.
ce9a070 to
e71588a
Compare
e71588a to
7267af5
Compare
ec69dfa to
4efd9d3
Compare
4efd9d3 to
5f79c97
Compare
|
Thanks all for the review! Great job @HeartLinked. Let me merge this to move forward. |
This PR introduces the foundational scaffolding for the Iceberg REST Catalog client implementation in C++. It establishes the core infrastructure for communicating with Iceberg REST Catalog servers following the Iceberg REST Catalog Specification.
Key Components Added
1. HTTP Client Infrastructure (
http_client.h/cc)HttpClientclass wrapping the CPR library for HTTP operations.GET,POST,POST(form-urlencoded),HEAD, andDELETEmethods.HttpResponsewrapper to abstract underlying HTTP library implementation.2. Configuration Management (
catalog_properties.h/cc)RestCatalogPropertiesclass for REST catalog configuration.3. Resource Path Construction (
resource_paths.h/cc)ResourcePathsclass for building REST API endpoint URLs.4. Error Handling Framework (
error_handlers.h/cc)result.hwith new rest related error kinds.5. REST Utilities (
rest_util.h/cc)0x1F).TrimTrailingSlash).7. RestCatalog Implementation (
rest_catalog.h/cc)RestCatalogclass implementing the Catalog interface./v1/config.Testing
rest_util_test.cc: Comprehensive tests for URL encoding/decoding, namespace encoding, config merging.rest_catalog_test.cc: These currently introduced tests are merely temporary integration tests that require a local REST server, such as theapache/iceberg-rest-fixtureDocker image While this enables local testing, it is incompatible with the GitHub CI process, so they have been marked asDISABLED_. In the future, we aim to follow the example of iceberg-rust by designing comprehensive integration tests to verify the REST catalog client's behavior and integrating them into our GitHub CI pipeline. This work is scheduled for later completion; please refer to issue Implement REST Catalog Integration Tests with Docker in CI Pipeline #333.