You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/15 19:56:54 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #4772: Core: Add request headers to REST client
rdblue opened a new pull request, #4772:
URL: https://github.com/apache/iceberg/pull/4772
This adds a parameter to `RESTClient` to pass headers when making a request. This was originally part of #4575, but we can get it in more easily on its own.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#discussion_r873218652
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java:
##########
@@ -96,6 +96,10 @@ public void initialize(String name, Map<String, String> props) {
this.io = CatalogUtil.loadFileIO(ioImpl != null ? ioImpl : ResolvingFileIO.class.getName(), properties, conf);
}
+ protected Map<String, String> headers() {
+ return ImmutableMap.of();
Review Comment:
Right now, headers aren't used other than overriding this in tests to validate that they can be set and are passed through the catalog correctly.
--
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
[GitHub] [iceberg] danielcweeks merged pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
danielcweeks merged PR #4772:
URL: https://github.com/apache/iceberg/pull/4772
--
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
[GitHub] [iceberg] rdblue commented on pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#issuecomment-1129151087
Thanks for the review, @danielcweeks! I think the `RequestContext` is a great idea, but I'm not sure that we're going to need it so I'd probably hold off and do it later.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#discussion_r873218790
##########
core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java:
##########
@@ -63,4 +63,8 @@ public String table(TableIdentifier ident) {
"v1", prefix, "namespaces", RESTUtil.encodeNamespace(ident.namespace()), "tables",
RESTUtil.encodeString(ident.name()));
}
+
+ public String rename() {
+ return SLASH.join("v1", prefix, "tables", "rename");
Review Comment:
The path was hard-coded for rename. I'm adding this here since this PR already modifies all the other lines to add headers. FYI @kbendick.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#discussion_r873218479
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -59,13 +59,12 @@ public class HTTPClient implements RESTClient {
private final String uri;
private final CloseableHttpClient httpClient;
private final ObjectMapper mapper;
- private final Map<String, String> additionalHeaders;
+ private final Map<String, String> baseHeaders;
- private HTTPClient(
- String uri, CloseableHttpClient httpClient, Map<String, String> additionalHeaders) {
+ private HTTPClient(String uri, Map<String, String> baseHeaders) {
this.uri = uri;
- this.httpClient = httpClient != null ? httpClient : HttpClients.createDefault();
Review Comment:
The builder was never initializing `httpClient` so it was always null anyway. This removes the unnecessary argument. FYI @kbendick.
--
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
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#discussion_r874234104
##########
core/src/main/java/org/apache/iceberg/rest/RESTClient.java:
##########
@@ -28,13 +29,15 @@
*/
public interface RESTClient extends Closeable {
Review Comment:
If we do decide to define a `RequestContext`, this might be a good place to add an inner interface.
--
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
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #4772: Core: Add request headers to REST client
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #4772:
URL: https://github.com/apache/iceberg/pull/4772#discussion_r874230116
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -195,33 +195,34 @@ private <T> T execute(
}
@Override
- public void head(String path, Consumer<ErrorResponse> errorHandler) {
- execute(Method.HEAD, path, null, null, errorHandler);
+ public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
+ execute(Method.HEAD, path, null, null, headers, errorHandler);
}
@Override
- public <T extends RESTResponse> T get(String path, Class<T> responseType,
+ public <T extends RESTResponse> T get(String path, Class<T> responseType, Map<String, String> headers,
Review Comment:
One thing you might want to consider is wrapping some of these parameters (i.e. headers and errorHandler) in some sort of wrapper that we can extend to further scope requests within sessions. Just thinking something on the order of:
```java
RequestContext {
Supplier<Map<String, String>> requestHeaders()
void handleResponseHeaders(Consumer<Map<String,String>>)
Consumer<ErrorResponse> errorHandler()
}
```
I feel like it will be hard to extend the current pattern if we need to propagate information back or retry requests (e.g. the 419 case) without being able to capture the current state.
--
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