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 2021/12/07 23:57:41 UTC
[GitHub] [iceberg] dimas-b commented on a change in pull request #3561: [CORE] Specification for an HTTP REST catalog
dimas-b commented on a change in pull request #3561:
URL: https://github.com/apache/iceberg/pull/3561#discussion_r764440889
##########
File path: core/src/main/java/org/apache/iceberg/rest/http/IcebergHttpResponse.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.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/**
+ * All responses for the first version of the Iceberg REST API will be JSON objects, a top-level envelope object with two fields,
+ * `error` and `data`, which are themselves JSON objects.
+ *
+ * data: represents the JSON encoded response for a successful path or for a valid / expected failure. JSON representation of various response types
+ * error: a standardized obect on error, containing the error code, message, type, and additional metadata (an optional JSON object of metadata) as defined below.
+ *
+ * All responses for the REST catalog should be wrapped this way, vs using primitives. For example, for a ListTableResponse, listing tables under a namespace "accounting",
+ * we'd get a JSON object back like the following:
+ *
+ * { "data": { "identifiers": [ "accounting.tax", "accounting.currency_conversions"] }, "error", {} }
Review comment:
nit: is an empty `error` object meaningful? Should it be `null` in this case, perhaps?
##########
File path: rest_docs/rest-catalog-open-api-v0.1.yaml
##########
@@ -0,0 +1,657 @@
+#
+# 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.
+#
+
+---
+openapi: 3.0.3
+info:
+ title: Apache Iceberg REST Catalog API
+ license:
+ name: Apache 2.0
+ url: https://www.apache.org/licenses/LICENSE-2.0.html
+ version: 1.0.0
+ description:
+ Defines the specification for the first version of the REST Catalog API. Implementations should support both Iceberg table specs v1 and v2, with priority given to v2.
+servers:
+ - url: https://{host}:{port}/{basePath}
+ variables:
+ host:
+ description: The host address for the specified server
+ default: localhost
+ port:
+ description: The port used when addressing the host
+ default: "443"
+ basePath:
+ default: v1
+ - url: http://127.0.0.1:1080/v1
+ description: URL Used for Mock-Server Unit Tests
+# All routes are currently configured using an Authorization header.
+security:
+ - BearerAuth: []
+paths:
+ /config:
+ get:
+ tags:
+ - Configuration API
+ summary: List all catalog configuration settings
+ operationId: getConfig
+ description: >
+ All REST catalog clients will first call this route to get possible catalog-specific
+ configuration values provided by the server, that the catalog (and its HTTP client)
+ can use to complete the `initialize` step.
+
+ This call is similar to the initial set-up calls that some catalogs already do for
+ domain-specific information, such as the Nessie catalog or the Glue catalog.
+ This is to allow for services that would like to integrate with Iceberg to do so,
+ and to be able to add their own domain-specific information into the REST catalog without
+ requiring them to write and distribute a catalog themselves.
+
+ There will be two sets of values provided -
+
+ - overrides
+ * An object containing values that the client must use.
+ For example, auth headers that the client will receive from the server
+ as temporary credentials.
+ - defaults
+ * Catalog-specific configuration that the client may use as a default value.
+ These are optional and the client is free to use its own value for these.
Review comment:
I'd suggest using the terms `normative` and `optional` instead. It is not clear what the "overrides" are overriding since the intention is obviously for these values to be the source of truth.
##########
File path: core/src/main/java/org/apache/iceberg/rest/http/IcebergHttpResponse.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.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/**
+ * All responses for the first version of the Iceberg REST API will be JSON objects, a top-level envelope object with two fields,
+ * `error` and `data`, which are themselves JSON objects.
+ *
+ * data: represents the JSON encoded response for a successful path or for a valid / expected failure. JSON representation of various response types
+ * error: a standardized obect on error, containing the error code, message, type, and additional metadata (an optional JSON object of metadata) as defined below.
+ *
+ * All responses for the REST catalog should be wrapped this way, vs using primitives. For example, for a ListTableResponse, listing tables under a namespace "accounting",
+ * we'd get a JSON object back like the following:
+ *
+ * { "data": { "identifiers": [ "accounting.tax", "accounting.currency_conversions"] }, "error", {} }
+ *
+ * If the namesapce `accounting` didn't request, the response from that call would have a body like the following,
+ * where the `code` 40401 is a two-part identifier:
+ * - HTTP response code: 404
+ * - Two digit internal application defined error code for further detail: 01 - Namespace not found.
+ *
+ * { "data": {}, "error": { "message": "Failed to list tables. The Namespace 'accounting' does not exist", "type": "NamespaceNotFoundException", "code": 40401 }
+ *
+ * We could also embed the HTTP response code plainly by itself, without internally documented codes, as a separate field. I have found having a documented list of internal
+ * error codes to be very helpful previously, but am open to discussion on this.
Review comment:
Could it be clearer if the HTTP error code were separate from the internal error code? This would allow free form internal code without having to be confined to 100 values under an HTTP code prefix, and at the same time maintain reference to the more generic HTTP set of codes. It would also make it easier to extract HTTP error codes for processing.
##########
File path: rest_docs/rest-catalog-open-api-v0.1.yaml
##########
@@ -0,0 +1,657 @@
+#
+# 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.
+#
+
+---
+openapi: 3.0.3
+info:
+ title: Apache Iceberg REST Catalog API
+ license:
+ name: Apache 2.0
+ url: https://www.apache.org/licenses/LICENSE-2.0.html
+ version: 1.0.0
+ description:
+ Defines the specification for the first version of the REST Catalog API. Implementations should support both Iceberg table specs v1 and v2, with priority given to v2.
+servers:
+ - url: https://{host}:{port}/{basePath}
+ variables:
+ host:
+ description: The host address for the specified server
+ default: localhost
+ port:
+ description: The port used when addressing the host
+ default: "443"
+ basePath:
+ default: v1
+ - url: http://127.0.0.1:1080/v1
+ description: URL Used for Mock-Server Unit Tests
+# All routes are currently configured using an Authorization header.
+security:
+ - BearerAuth: []
+paths:
+ /config:
+ get:
+ tags:
+ - Configuration API
+ summary: List all catalog configuration settings
+ operationId: getConfig
+ description: >
+ All REST catalog clients will first call this route to get possible catalog-specific
+ configuration values provided by the server, that the catalog (and its HTTP client)
+ can use to complete the `initialize` step.
+
+ This call is similar to the initial set-up calls that some catalogs already do for
+ domain-specific information, such as the Nessie catalog or the Glue catalog.
+ This is to allow for services that would like to integrate with Iceberg to do so,
+ and to be able to add their own domain-specific information into the REST catalog without
+ requiring them to write and distribute a catalog themselves.
+
+ There will be two sets of values provided -
+
+ - overrides
+ * An object containing values that the client must use.
+ For example, auth headers that the client will receive from the server
+ as temporary credentials.
+ - defaults
+ * Catalog-specific configuration that the client may use as a default value.
+ These are optional and the client is free to use its own value for these.
Review comment:
By the way, what is a use case for the "default" properties? Could you add an example, please?
##########
File path: core/src/main/java/org/apache/iceberg/rest/http/IcebergHttpResponse.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.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/**
+ * All responses for the first version of the Iceberg REST API will be JSON objects, a top-level envelope object with two fields,
+ * `error` and `data`, which are themselves JSON objects.
+ *
+ * data: represents the JSON encoded response for a successful path or for a valid / expected failure. JSON representation of various response types
+ * error: a standardized obect on error, containing the error code, message, type, and additional metadata (an optional JSON object of metadata) as defined below.
+ *
+ * All responses for the REST catalog should be wrapped this way, vs using primitives. For example, for a ListTableResponse, listing tables under a namespace "accounting",
+ * we'd get a JSON object back like the following:
+ *
+ * { "data": { "identifiers": [ "accounting.tax", "accounting.currency_conversions"] }, "error", {} }
+ *
+ * If the namesapce `accounting` didn't request, the response from that call would have a body like the following,
+ * where the `code` 40401 is a two-part identifier:
+ * - HTTP response code: 404
+ * - Two digit internal application defined error code for further detail: 01 - Namespace not found.
+ *
+ * { "data": {}, "error": { "message": "Failed to list tables. The Namespace 'accounting' does not exist", "type": "NamespaceNotFoundException", "code": 40401 }
+ *
+ * We could also embed the HTTP response code plainly by itself, without internally documented codes, as a separate field. I have found having a documented list of internal
+ * error codes to be very helpful previously, but am open to discussion on this.
+ */
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class IcebergHttpResponse<T> {
+
+ private final T data;
+ private final Error error;
+
+ @JsonCreator
+ public IcebergHttpResponse(
+ @JsonProperty("data") T data,
+ @JsonProperty("error") Error error) {
+ this.data = data;
+ this.error = error;
+ }
+
+ public Error error() {
+ return error;
+ }
+
+ public T data() {
+ return data;
+ }
+
+ /**
+ * An error object embedded in every HTTP response.
+ *
+ * On error, this contains:
+ * - message: A short, human-readable description of the error.
+ * - type: Type of exception - more specifically a class name, e.g. NamespaceNotFoundException)
Review comment:
Does this have to be part of the spec? In other words are specific exception class names going to be part of the spec in addition and/or in parallel to "internal" error codes?
--
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