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/02/11 21:05:36 UTC
[GitHub] [iceberg] kbendick opened a new pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
kbendick opened a new pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097
This is a basic HTTP client for usage with the REST catalog.
Opening this PR to get feedback on the direction that this is heading, as well as to be able to test end to end the full catalog (coming in the next PR).
Some of the requests and the serializers have been taken from the other PR for use with testing.
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807300504
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
Review comment:
Yeah I'll remove the default case from the others.
--
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] kbendick commented on pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#issuecomment-1048391443
This has been split up, to unblock parallel work on some of the test harnesses and other things that are needed.
See:
- https://github.com/apache/iceberg/pull/4201
- https://github.com/apache/iceberg/pull/4202
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r811461579
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
The JAR itself is about 779 kb (so not small)
And then it has the following compile time dependencies as well
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810317905
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
I don't think this is included with Spark. Let me double check.
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805031055
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest.responses;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+/**
+ * Standard response body on error.
+ */
+public class ErrorResponse {
+
+ private final String message;
+ private final String type;
+ private final Integer code;
+
+ // These aren't part of the spec, but they are likely needed when we abstract out the current HTTP client.
+ @JsonIgnore
+ private final String body;
+ @JsonIgnore
+ private Map<String, String> headers;
+
+ private ErrorResponse(String message, String type, Integer code) {
+ this(message, type, code, null, Collections.emptyMap());
+ }
+
+ private ErrorResponse(String message, String type, Integer code, String body, Map<String, String> headers) {
+ this.message = message;
+ this.type = type;
+ this.code = code;
+ this.body = body;
+ this.headers = headers;
+ }
+
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ public String message() {
+ return message;
+ }
+
+ public String type() {
+ return type;
+ }
+
+ public Integer code() {
+ return code;
+ }
+
+ public String body() {
+ return body;
+ }
Review comment:
This is the raw response body. It's not part of the current `ErrorModel` definition, but I think having it in case the response that returns an error isn't a proper `ErrorResponse`.
For example, if a request timesout before ever hitting the server, it can return `text/plain` in which case having the body would be useful.
I'd likely consider moving the raw body and the headers to a `ResponseContext` object so that they can be grabbed from the response type pulled from the implementation.
--
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 change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810541391
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
How large is this dependency if we add it? We might just want to add and shade it everywhere if it isn't simple to rely on the one already in the classpath.
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805026872
##########
File path: core/src/main/java/org/apache/iceberg/rest/responses/CreateNamespaceResponse.java
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest.responses;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+
+/**
+ * Represents a REST response for a request to create a namespace / database.
+ * <p>
+ * The properties returned should include all the user provided properties from the
+ * request, as well as any server-side added properties.
+ * <p>
+ * Example server-side added properties can be implementation specific, but might include
+ * such things as `created_at` or `owner`.
+ * <p>
+ * NOTE - This is copied over from the SerDe PR and should be removed before merging.
+ */
+public class CreateNamespaceResponse {
Review comment:
This file is taken from the other PR and can be ignored here,
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805026753
##########
File path: core/src/main/java/org/apache/iceberg/rest/requests/CreateNamespaceRequest.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest.requests;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+/**
+ * A REST request to create a namespace, with an optional set of properties.
+ * The server might also add properties, such as `last_modified_time` etc.
+ * <p>
+ * NOTE - This is copied over from the SerDe PR and should be removed before merging.
+ */
+public class CreateNamespaceRequest {
+
+ private Namespace namespace;
+ private Map<String, String> properties;
+
+ private CreateNamespaceRequest(@JsonProperty("namespace") Namespace namespace,
+ @JsonProperty("properties") Map<String, String> properties) {
+ Preconditions.checkArgument(namespace != null && !namespace.isEmpty(),
+ "Cannot build a request to create a namespace without specifying the namespace");
+ this.namespace = namespace;
+ this.properties = properties != null ? properties : Collections.emptyMap();
+ }
+
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ public Namespace namespace() {
+ return namespace;
+ }
+
+ public Map<String, String> properties() {
+ return ImmutableMap.copyOf(properties);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("namespace", namespace)
+ .add("properties", properties)
+ .toString();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ CreateNamespaceRequest request = (CreateNamespaceRequest) o;
+ return namespace.equals(request.namespace) && Objects.equals(properties, request.properties);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(namespace, properties);
+ }
+
+ public static class Builder {
+ private Namespace namespace;
+ private Map<String, String> properties = Maps.newHashMap();
+
+ private Builder() {
+ }
+
+ public Builder withNamespace(Namespace ns) {
+ this.namespace = ns;
+ return this;
+ }
+
+ public Builder setProperties(Map<String, String> props) {
+ properties.putAll(props);
+ return this;
+ }
+
+ public CreateNamespaceRequest build() {
+ return new CreateNamespaceRequest(namespace, properties);
+ }
+ }
+}
Review comment:
This file is taken from the other PR and can be ignored here,
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810324245
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
Review comment:
Removing this from this table error handler as it's in the default handler, and we can chain them via `namespaceErrorHandler.andThen(defaultErrorHandler)`.
The `defaultErrorHandler` does differentiate between the two cases.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807130701
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
This is already included with Spark, right? So we should make sure that it is excluded from the Spark runtime Jars.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807143143
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTClient.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.Closeable;
+import java.util.function.Consumer;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * Interface for a basic HTTP Client for interfacing with the REST catalog.
+ */
+public interface RESTClient extends Closeable {
Review comment:
Should this have a method that can set the default error handler?
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810321818
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
Review comment:
Actually, even for table related resources, we can wind up with a NamespaceNotFound response.
For example, `CreateTableRequest` can return a NamespaceNotFound 404 if the namespace to create the table in doesn't exist: https://github.com/apache/iceberg/blob/9cc2ac8ec7172c78510bf4ddbcda9d9bc7b5a31c/docs/rest/rest-catalog-open-api.yaml#L345-L384
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810327874
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
Review comment:
The default case has been removed from the others. I also made the others private, renamed them to `baseTableErrorHandler` and `baseNamespaceErrorHandler`, and used the original names to export already chained error handlers.
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805033907
##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -49,6 +52,17 @@ public static ObjectMapper mapper() {
return MAPPER;
}
+ public static ObjectMapper withRESTComponents() {
+ RESTSerializers.registerAll(MAPPER);
+ // These are a workaround for Jackson since Iceberg doesn't use the standard get/set bean notation.
+ // This allows Jackson to work with the fields directly (both public and private) and not require
+ // custom (de)serializers for all the request/response objects.
+ MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+ MAPPER.setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY); // deserialization
+ MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); // serialization
+ return MAPPER;
+ }
+
Review comment:
These changes are part of the other PR and so should be reviewed there. They're included here for test purposes.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807141144
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
+ }
+ };
+ }
+
+ static String extractResponseBodyAsString(CloseableHttpResponse response) {
Review comment:
Is this used? I thought this part of the API was going to avoid depending on a specific HTTP library.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807140284
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
Review comment:
Both this handler and the others throw in a default case, so they can't be chained. I think you want to chain them, right?
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807302709
##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -49,6 +52,17 @@ public static ObjectMapper mapper() {
return MAPPER;
}
+ public static ObjectMapper withRESTComponents() {
+ RESTSerializers.registerAll(MAPPER);
+ // These are a workaround for Jackson since Iceberg doesn't use the standard get/set bean notation.
+ // This allows Jackson to work with the fields directly (both public and private) and not require
+ // custom (de)serializers for all the request/response objects.
+ MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+ MAPPER.setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY); // deserialization
+ MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); // serialization
+ return MAPPER;
+ }
+
Review comment:
I didn't think we really came to a conclusion about removing these. I know we talked about removing annotations (`@JsonProperty`).
The tests can definitely instantiate their own mappers, but without these somewhere we'll need to write out the Serializer / Deserializer for all of these (or make the fields public - which even then some mappers might be able to turn these off).
I'll move these to the test classes for now though.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810328958
##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -49,6 +52,17 @@ public static ObjectMapper mapper() {
return MAPPER;
}
+ public static ObjectMapper withRESTComponents() {
+ RESTSerializers.registerAll(MAPPER);
+ // These are a workaround for Jackson since Iceberg doesn't use the standard get/set bean notation.
+ // This allows Jackson to work with the fields directly (both public and private) and not require
+ // custom (de)serializers for all the request/response objects.
+ MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+ MAPPER.setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY); // deserialization
+ MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); // serialization
+ return MAPPER;
+ }
+
Review comment:
Dealt with this in the other PR. Will be removing this from this PR entirely.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807130701
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
This is already included with Spark, right? So we should make sure that it is excluded from the Spark runtime Jars.
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
Review comment:
For HEAD requests, this should simply throw `NoSuchTableException` because the request is specifically for the table.
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
Review comment:
What does this comment mean? Isn't this what `SC_UNPROCESSABLE_ENTITY` means? ([spec](https://github.com/apache/iceberg/blob/master/docs/rest/rest-catalog-open-api.yaml#L303))
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
Review comment:
There is an exception class for this. Should this throw it?
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
Review comment:
Both this handler and the others throw in a default case, so they can't be chained. I think you want to chain them, right?
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
+ }
+ };
+ }
+
+ static String extractResponseBodyAsString(CloseableHttpResponse response) {
Review comment:
Is this used? I thought this part of the API was going to avoid depending on a specific HTTP library.
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
+ }
+ };
+ }
+
+ static String extractResponseBodyAsString(CloseableHttpResponse response) {
Review comment:
Looks like this should be moved into `HttpRESTClient`.
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
Review comment:
I think this and forbidden should be handled by the default handler.
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTClient.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.Closeable;
+import java.util.function.Consumer;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * Interface for a basic HTTP Client for interfacing with the REST catalog.
+ */
+public interface RESTClient extends Closeable {
Review comment:
Should this have a method that can set the default error handler?
##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -49,6 +52,17 @@ public static ObjectMapper mapper() {
return MAPPER;
}
+ public static ObjectMapper withRESTComponents() {
+ RESTSerializers.registerAll(MAPPER);
+ // These are a workaround for Jackson since Iceberg doesn't use the standard get/set bean notation.
+ // This allows Jackson to work with the fields directly (both public and private) and not require
+ // custom (de)serializers for all the request/response objects.
+ MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+ MAPPER.setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY); // deserialization
+ MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); // serialization
+ return MAPPER;
+ }
+
Review comment:
I thought we talked about removing these. Can't the tests instantiate their own mapper?
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807141703
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
+ }
+ };
+ }
+
+ static String extractResponseBodyAsString(CloseableHttpResponse response) {
Review comment:
Looks like this should be moved into `HttpRESTClient`.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807139280
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
Review comment:
What does this comment mean? Isn't this what `SC_UNPROCESSABLE_ENTITY` means? ([spec](https://github.com/apache/iceberg/blob/master/docs/rest/rest-catalog-open-api.yaml#L303))
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807137485
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
Review comment:
For HEAD requests, this should simply throw `NoSuchTableException` because the request is specifically for the table.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810322493
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
Review comment:
And a table rename can return either 404 table not found or 404 namespace not found (so it might eventually need its own error handler): https://github.com/apache/iceberg/blob/9cc2ac8ec7172c78510bf4ddbcda9d9bc7b5a31c/docs/rest/rest-catalog-open-api.yaml#L545-L597
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805026329
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
##########
@@ -0,0 +1,77 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.JsonSerializer;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import java.io.IOException;
+import org.apache.iceberg.catalog.Namespace;
+
+public class RESTSerializers {
+
+ private RESTSerializers() {
+ }
+
+ public static void registerAll(ObjectMapper mapper) {
+ SimpleModule module = new SimpleModule();
+ module
+ .addSerializer(Namespace.class, new NamespaceSerializer())
+ .addDeserializer(Namespace.class, new NamespaceDeserializer());
+ mapper.registerModule(module);
+ }
+
+ public static class NamespaceDeserializer extends JsonDeserializer<Namespace> {
+ @Override
+ public Namespace deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
+ JsonNode jsonNode = p.getCodec().readTree(p);
+ return namespaceFromNode(p, jsonNode);
+ }
+ }
+
+ public static class NamespaceSerializer extends JsonSerializer<Namespace> {
+ @Override
+ public void serialize(Namespace namespace, JsonGenerator gen, SerializerProvider serializers)
+ throws IOException {
+ String[] parts = namespace.levels();
+ gen.writeArray(parts, 0, parts.length);
+ }
+ }
+
+ private static Namespace namespaceFromNode(JsonParser parser, JsonNode jsonNode) throws IOException {
+ if (!jsonNode.isArray()) {
+ throw new JsonParseException(parser, "Namespace must be an array: " + jsonNode);
+ }
+ ArrayNode arrayNode = (ArrayNode) jsonNode;
+ String[] levels = new String[arrayNode.size()];
+ for (int i = 0; i < levels.length; i++) {
+ levels[i] = arrayNode.get(i).asText();
+ }
+ return Namespace.of(levels);
+ }
+}
Review comment:
This is taken from the other PR and can be ignored here,
--
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] kbendick commented on a change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r805026329
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
##########
@@ -0,0 +1,77 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.JsonSerializer;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import java.io.IOException;
+import org.apache.iceberg.catalog.Namespace;
+
+public class RESTSerializers {
+
+ private RESTSerializers() {
+ }
+
+ public static void registerAll(ObjectMapper mapper) {
+ SimpleModule module = new SimpleModule();
+ module
+ .addSerializer(Namespace.class, new NamespaceSerializer())
+ .addDeserializer(Namespace.class, new NamespaceDeserializer());
+ mapper.registerModule(module);
+ }
+
+ public static class NamespaceDeserializer extends JsonDeserializer<Namespace> {
+ @Override
+ public Namespace deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
+ JsonNode jsonNode = p.getCodec().readTree(p);
+ return namespaceFromNode(p, jsonNode);
+ }
+ }
+
+ public static class NamespaceSerializer extends JsonSerializer<Namespace> {
+ @Override
+ public void serialize(Namespace namespace, JsonGenerator gen, SerializerProvider serializers)
+ throws IOException {
+ String[] parts = namespace.levels();
+ gen.writeArray(parts, 0, parts.length);
+ }
+ }
+
+ private static Namespace namespaceFromNode(JsonParser parser, JsonNode jsonNode) throws IOException {
+ if (!jsonNode.isArray()) {
+ throw new JsonParseException(parser, "Namespace must be an array: " + jsonNode);
+ }
+ ArrayNode arrayNode = (ArrayNode) jsonNode;
+ String[] levels = new String[arrayNode.size()];
+ for (int i = 0; i < levels.length; i++) {
+ levels[i] = arrayNode.get(i).asText();
+ }
+ return Namespace.of(levels);
+ }
+}
Review comment:
This file is taken from the other PR and can be ignored here,
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810327968
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handler that handles the common cases that are included with all responses,
+ * such as 400, 401, 403, 500, etc.
+ */
+ public static Consumer<ErrorResponse> defaultErrorHandler() {
+ return errorResponse -> {
+ switch (errorResponse.code()) {
+ // 400
+ case HttpStatus.SC_CLIENT_ERROR:
+ throw new BadRequestException("Unable to process the request as it is somehow malformed: %s", errorResponse);
+ // 401
+ case HttpStatus.SC_UNAUTHORIZED:
+ throw new NotAuthorizedException("Not Authorized: %s", errorResponse);
+ // 403
+ case HttpStatus.SC_FORBIDDEN:
+ throw new ForbiddenException("Forbidden: %s", errorResponse);
+ // 405 & 406 & 501 - All of these can represent not supported.
+ case HttpStatus.SC_METHOD_NOT_ALLOWED:
+ case HttpStatus.SC_NOT_ACCEPTABLE:
+ case HttpStatus.SC_NOT_IMPLEMENTED:
+ throw new UnsupportedOperationException(String.format("Not supported: %s", errorResponse));
+ // 500
+ case HttpStatus.SC_SERVER_ERROR:
+ throw new ServiceFailureException("Server error: %s", errorResponse);
+ default:
+ throw new RESTException("Unknown error: %s", errorResponse);
+ }
+ };
+ }
+
+ static String extractResponseBodyAsString(CloseableHttpResponse response) {
Review comment:
Will move this.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810323485
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
Review comment:
The comment is mostly to clarify why the exception message is so specific, but I've removed it.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810323485
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
Review comment:
The comment is mostly to clarify why the exception message is so specific.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810319854
##########
File path: build.gradle
##########
@@ -221,6 +223,8 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ implementation 'org.apache.httpcomponents.client5:httpclient5'
Review comment:
`spark_hive` has an older version, which comes with a different module name. `org.apache.httpcomponents:httpclient:4.5.13` for Spark 3.2.1: https://mvnrepository.com/artifact/org.apache.spark/spark-hive_2.12/3.2.1
We might need to exclude some packages, but I think they've mostly all been renamed as of the name change to include `5` in the artifact name and the org.
--
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] kbendick commented on a change in pull request #4097: REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r810327148
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
Review comment:
Updated.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807139529
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
Review comment:
There is an exception class for this. Should this throw it?
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807142516
##########
File path: core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.RESTException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A set of consumers to handle errors for requests for table entities or for namespace entities,
+ * to throw the correct exception.
+ */
+public class ErrorHandlers {
+
+ private ErrorHandlers() {
+ }
+
+ /**
+ * Table level error handlers.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> tableErrorHandler() {
+ return error -> {
+
+ switch (error.code()) {
+ // table resource routes can encounter namespace not found or table not found.
+ // TODO - Need to define response types (can probably use the HTTP values and specialize from there).
+ case HttpStatus.SC_NOT_FOUND:
+ // TODO - error.type() could be null for HEAD responses. We probably want to pass
+ // in some request info using a BiConsumer so we can have the table name etc
+ // from the request.
+ if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
+ throw new NoSuchNamespaceException("Resource not found: %s", error);
+ } else {
+ throw new NoSuchTableException("Resource not found: %s", error);
+ }
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("The table already exists: %s", error);
+ default:
+ throw new RESTException("Unknown error: %s", error);
+ }
+ };
+ }
+
+ /**
+ * Request error handlers specifically for CRUD ops on namespaces.
+ * Should be chained wih the {@linkplain ErrorHandlers#defaultErrorHandler}, which takes care of common cases.
+ */
+ public static Consumer<ErrorResponse> namespaceErrorHandler() {
+ return error -> {
+ switch (error.code()) {
+ case HttpStatus.SC_NOT_FOUND:
+ throw new NoSuchNamespaceException("Namespace not found: %s", error);
+ case HttpStatus.SC_CONFLICT:
+ throw new AlreadyExistsException("Namespace already exists: %s", error);
+ // Invalid UpdateNamespacePropertiesRequest - keys to remove overlapped with keys of properties to add
+ case HttpStatus.SC_UNPROCESSABLE_ENTITY:
+ throw new RESTException("Unable to process request due to bad property updates");
+ case HttpStatus.SC_FORBIDDEN:
+ case HttpStatus.SC_UNAUTHORIZED:
Review comment:
I think this and forbidden should be handled by the default handler.
--
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 change in pull request #4097: [WIP] REST Catalog - Initial Http Client Interface & Basic Implementation
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4097:
URL: https://github.com/apache/iceberg/pull/4097#discussion_r807144426
##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -49,6 +52,17 @@ public static ObjectMapper mapper() {
return MAPPER;
}
+ public static ObjectMapper withRESTComponents() {
+ RESTSerializers.registerAll(MAPPER);
+ // These are a workaround for Jackson since Iceberg doesn't use the standard get/set bean notation.
+ // This allows Jackson to work with the fields directly (both public and private) and not require
+ // custom (de)serializers for all the request/response objects.
+ MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+ MAPPER.setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY); // deserialization
+ MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); // serialization
+ return MAPPER;
+ }
+
Review comment:
I thought we talked about removing these. Can't the tests instantiate their own mapper?
--
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