Add support for Iceberg catalog warehouse property in REST #15762

danielcweeks · 2023-01-18T17:02:49Z

Description

This PR adds support for configuring the warehouse catalog property for the REST Catalog which is part of the REST Spec.

Additional context and related issues

N/A

Release notes

(X) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

...ino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogConfig.java

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java

krvikash · 2023-01-19T06:16:02Z

Hi @danielcweeks, Could you check the CI failure?

findepi · 2023-01-19T10:01:58Z

LGTM

nastra · 2023-01-19T10:53:15Z

I've fixed CI failures and tests should pass now

krvikash

LGTM. CC: @colebow for the doc review.

findepi · 2023-01-19T12:30:46Z

docs/src/main/sphinx/connector/iceberg.rst

@@ -95,6 +95,9 @@ Property Name                                        Description
 ``iceberg.rest-catalog.uri``                   REST server API endpoint URI (required).
                                               Example: ``http://iceberg-with-rest:8181``

+``iceberg.rest-catalog.warehouse``             Warehouse identifier/location for the catalog (optional).


in JDBC catalog we call it iceberg.jdbc-catalog.default-warehouse-dir
with description "The default warehouse directory to use for JDBC"

the "identifier" is misleading
the config name is unnecessarily different

@mosabua do we write "(optional)" for optional properties, or just for required ones (as above), or ?

findepi · 2023-01-19T12:31:46Z

...ino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogConfig.java

+    @Config("iceberg.rest-catalog.warehouse")
+    @ConfigDescription("The warehouse location/identifier to use with the REST catalog server")
+    public IcebergRestCatalogConfig setWarehouse(String warehouse)
+    {
+        this.warehouse = Optional.ofNullable(warehouse);


Let's sync this with io.trino.plugin.iceberg.catalog.jdbc.IcebergJdbcCatalogConfig#setDefaultWarehouseDir

(config name, description, property name, field name)

Similar to the comment below, the REST catalog makes this a little more flexible and doesn't necessarily require an actual directory (it could just be a name that the catalog maps to a directory).

findepi · 2023-01-19T12:32:00Z

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java

@@ -38,6 +39,7 @@
    private final CatalogName catalogName;
    private final String trinoVersion;
    private final URI serverUri;
+    private final Optional<String> warehouse;


defaultWarehouseDir ?

@findepi this isn't exactly the same thing as with JDBC since the warehouse property isn't required to be a directory (it could just be an identifier that may map to a location/bucket/etc.). The property itself is really just 'warehouse' and we probably don't want to restrict that here.

what is warehouse identifier?

are there any docs for that that we should reference from our docs?

An identifier could be anything that the REST Server would use to map to the warehouse being addressed. This could be as simple as something like prod which the server then would map to whatever the canonical location is for prod. For the docs, we could just reference the spec, but there's no strict requirement on format or how a server uses the value.

A good example of why this is useful is if you have something like the JDBCCatalog where you have to explicitly define the location in the config, but if something like Flink is also using the same catalog backend, but unintentionally points to a different location, you have a single catalog with data that's split across multiple locations. This can even result in individual tables having data end up split across multiple locations depending on writes/overwrites.

ebyhr · 2023-01-19T23:54:43Z

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java

@@ -71,6 +74,7 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity)
        if (icebergCatalog == null) {
            ImmutableMap.Builder<String, String> properties = ImmutableMap.builder();
            properties.put(CatalogProperties.URI, serverUri.toString());
+            warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location));


@danielcweeks I set random characters with iceberg.rest-catalog.warehouse in IcebergRestQueryRunnerMain, but I didn't find any change. I expected the REST catalog throws an error or log something because the behind JDBC catalogs uses the different warehouse catalog property. Can you share how REST catalog behaves in this case?

@ebyhr this is how the REST catalog currently uses this property: https://github.com/apache/iceberg/blob/4b5ee1a0ce77161b9195d8db4fe0e8350ea93fa8/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L736-L746.

Thanks, but the link doesn't answer the above question in my opinion. What's the expected behavior in the above random warehouse case?

@ebyhr I tried to explain in this comment, but this can't really be compared to how Hive works with locations and the expectation is not that the value should necessarily dictate anything about the path.

Because Iceberg has multiple implementations for Catalog, all with different behaviors, the warehouse property is just a standard mechanism by which to indicate intent to the implementation. For simple implementations, like Hadoop or Hive Catalog, that could be just a physical location to store data. However, it is more of an abstraction that can be used by the implementation.

For example, the catalog service at Netflix has multiple catalogs (prod, test, shadow, etc.), but there's just a single service endpoint (say metacat.data.netflix.net). To address each of those independently, you need to either have different domains for each or expose different ports to the same service. With RESTCatalog implementation, you're just providing a request to address a specific one without having to change the surrounding infrastructure (and something like prod is a lot easier to reason about than port 337033).

The REST spec defines this as an option, but does not dictate how it is handled (and ignoring it is also an option, since that is a catalog behavior).

We could write tests, but at that point we'd be writing both the test and the expected behavior, so the test would largely be meaningless.

findepi · 2023-01-20T09:12:41Z

@nastra FYI i push empty commit called empty when i want a CI re-run.

github-actions · 2023-02-20T20:53:51Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

danielcweeks · 2023-02-20T23:42:29Z

@findepi or @ebyhr is there anything blocking to get this in?

bitsondatadev · 2023-02-21T20:35:23Z

@findepi is currently on vacation, @ebyhr any thoughts on this being merge ready?

cla-bot bot added the cla-signed label Jan 18, 2023

krvikash reviewed Jan 18, 2023

View reviewed changes

...ino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogConfig.java Outdated Show resolved Hide resolved

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java Outdated Show resolved Hide resolved

github-actions bot added the docs label Jan 18, 2023

Add support for Iceberg catalog warehouse property in REST

8fa7519

nastra force-pushed the iceberg-catalog-warehouse-property branch from c4cdc9e to 8fa7519 Compare January 19, 2023 10:52

krvikash approved these changes Jan 19, 2023

View reviewed changes

findepi approved these changes Jan 19, 2023

View reviewed changes

findepi requested a review from ebyhr January 19, 2023 12:34

ebyhr reviewed Jan 19, 2023

View reviewed changes

nastra closed this Jan 20, 2023

nastra reopened this Jan 20, 2023

empty

27577c4

github-actions bot added the stale label Feb 20, 2023

github-actions bot removed the stale label Feb 21, 2023

ebyhr approved these changes Feb 21, 2023

View reviewed changes

ebyhr merged commit fba39d6 into trinodb:master Feb 21, 2023

github-actions bot added this to the 408 milestone Feb 22, 2023

colebow mentioned this pull request Feb 22, 2023

Add Trino 408 release notes #16209

Merged

danielcweeks deleted the iceberg-catalog-warehouse-property branch February 24, 2023 17:55

grantatspothero mentioned this pull request Mar 6, 2023

Iceberg REST catalog silently ignores 'location' schema and table property #16394

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Iceberg catalog warehouse property in REST #15762

Add support for Iceberg catalog warehouse property in REST #15762

danielcweeks commented Jan 18, 2023

krvikash commented Jan 19, 2023

findepi commented Jan 19, 2023

nastra commented Jan 19, 2023

krvikash left a comment

findepi Jan 19, 2023

findepi Jan 19, 2023

findepi Jan 19, 2023

danielcweeks Jan 19, 2023

findepi Jan 19, 2023

danielcweeks Jan 19, 2023

findepi Jan 20, 2023

danielcweeks Jan 20, 2023

ebyhr Jan 19, 2023

nastra Jan 20, 2023

ebyhr Feb 21, 2023 •

edited

Loading

danielcweeks Feb 21, 2023 •

edited

Loading

findepi commented Jan 20, 2023

github-actions bot commented Feb 20, 2023

danielcweeks commented Feb 20, 2023

bitsondatadev commented Feb 21, 2023

Add support for Iceberg catalog warehouse property in REST #15762

Add support for Iceberg catalog warehouse property in REST #15762

Conversation

danielcweeks commented Jan 18, 2023

Description

Additional context and related issues

Release notes

krvikash commented Jan 19, 2023

findepi commented Jan 19, 2023

nastra commented Jan 19, 2023

krvikash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

danielcweeks Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

findepi commented Jan 20, 2023

github-actions bot commented Feb 20, 2023

danielcweeks commented Feb 20, 2023

bitsondatadev commented Feb 21, 2023

ebyhr Feb 21, 2023 •

edited

Loading

danielcweeks Feb 21, 2023 •

edited

Loading