Core: Add RESTCatalog and RESTTableOperations #4348

rdblue · 2022-03-16T18:27:06Z

This adds RESTCatalog and RESTTableOperations implementations that are passing CatalogTests from #4319. This is the remainder of the changes from #4241.

In addition, this adds CatalogHandlers that implements the REST server operations that accept request classes, call Catalog, and produce response classes. This class can be used to implement REST catalog services that are backed by a Catalog, like JdbcCatalog. In this PR, the handlers are used to test RESTCatalog and RESTTableOperations along with a test class that implements RESTClient and calls the correct handler.

There are some features of the REST catalog that are missing and will be done in follow-up PRs:

The config route is not called to configure the catalog
The purge flag is not passed to the catalog service
The parent query param for namespace listing is not passed to the catalog service
Rename is not implemented

To support create transactions in CatalogHandlers, this exposes the table operations and starting metadata in BaseTransaction.

rdblue · 2022-03-16T18:34:30Z

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

+          .setProperties(properties)
+          .build();
+
+      // TODO: will this be a specific route or a modified create?


Right now, this uses a new route to state a table create. I haven't added this to the REST spec yet because it is an open question whether we want to use the existing create route with a query param or request flag, or use a separate route like this. Any opinions?

I personally vote for query param. It's almost an entire duplication of the create route if we go with a separate endpoint.

rdblue · 2022-03-16T18:37:58Z

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

+    UpdateTableRequest request = requestBuilder.build();
+
+    // the error handler will throw necessary exceptions like CommitFailedException and UnknownCommitStateException
+    // TODO: ensure that the HTTP client lib passes HTTP client errors to the error handler


@kbendick, this is relevant to the HTTP implementation of RESTClient. How does it handle errors that are coming from the HTTP connection and not from the service? In some cases, we need to throw CommitStateUnknownException.

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

RussellSpitzer · 2022-03-16T21:25:32Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+
+    Map<String, String> startProperties = catalog.loadNamespaceMetadata(namespace);
+    Set<String> missing = Sets.difference(removals, startProperties.keySet());
+


Another potential validation here, we are allowed to request the removal of a property that is not set, is that ok? Seems fine to me since I think a lot of property structures let you remove a non-existing key without error.

The idea is for these to be idempotent, so it is fine to request removing a property that is not set.

If we didn't allow that, then you'd get incorrect errors when a user does something like SHOW TBLPROPERTIES followed by concurrent UNSET commands. Retries would also cause problems.

RussellSpitzer · 2022-03-16T21:29:53Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+    return UpdateNamespacePropertiesResponse.builder()
+            .addMissing(missing)
+            .addUpdated(updates.keySet())
+            .addRemoved(Sets.difference(removals, missing))


This may not be true at this point since (as your comment alludes to above) we may have removed properties that were set in between our determination of missing and our execution of catalog.removeProperties. Not sure if we can make a better response though unless we change catalog.removeProperties to return which properties actually got removed.

This is something for implementations to improve on. Right now there's no way to do this through the catalog API.

RussellSpitzer · 2022-03-16T21:36:17Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+
+    TableMetadata metadata = TableMetadata.newTableMetadata(
+        request.schema(),
+        request.spec() != null ? request.spec() : PartitionSpec.unpartitioned(),


I wonder if we should just add these to the "spec() and writeOrder()" methods rather than null checking here.

I don't know. I'm reluctant to make the request return defaults instead of null. I'd probably leave this as it is here and let the implementation decide how to handle requests without these. That, or update newTableMetadata to do it.

RussellSpitzer · 2022-03-16T21:51:11Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+    TableMetadata finalMetadata;
+    if (isCreate(request)) {
+      // this is a hacky way to get TableOperations for an uncommitted table
+      Transaction transaction = catalog.buildTable(ident, EMPTY_SCHEMA).createOrReplaceTransaction();


Is the plan here that we end up making a replace table transaction here which replaces the newly created table with the updates? I'm not quite sure I follow this branch

Replace transactions are already working (using assertions and the normal commit path).

What's happening here is that the existing Catalog implementations don't allow you to get TableOperations. For all other commits, we load the table and grab TableOperations from BaseTable (see line 236). But for a create transaction, there is no table to load. The way I found to create TableOperations for a table that doesn't exist yet is to start a create transaction and get TableOperations from there.

As the comment says, this is a hacky way to get TableOperations, but I don't have a better one. Other options are: (1) don't support create transactions when wrapping Catalog, or (2) expose TableOperations in Catalog and force implementations to add a factory method. I think this is the cleanest way.

RussellSpitzer · 2022-03-16T22:04:56Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+          .filter(req -> !(req instanceof UpdateTableRequest.UpdateRequirement.AssertTableDoesNotExist))
+          .collect(Collectors.toList());
+      Preconditions.checkArgument(invalidRequirements.isEmpty(),
+          "Invalid create requirements: %s", invalidRequirements);


So either all requirements assert that the table does not exist, or none do? If only some do we have an invalid Create request?

Yes, that's correct.

If you're asserting that the table does not exist, then it makes no sense to assert that it has a certain current schema ID, for example.

RussellSpitzer

Went through CatalogHandlers, Will try to get through the rest later

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

core/src/main/java/org/apache/iceberg/BaseTransaction.java

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

rdblue · 2022-03-20T22:22:04Z

Since this is already large and close to being committed, I plan to update the code for #4366 once this is in.

kbendick · 2022-03-21T03:32:39Z

api/src/main/java/org/apache/iceberg/exceptions/UnprocessableEntityException.java

+ * <p>
+ * For example, this is used when a property update requests that properties are simultaneously set and removed.
+ */
+public class UnprocessableEntityException extends RESTException {


It might make sense to mention that this is HTTP 422. Somewhat implied by the name as well as extending RESTException though.

api/src/main/java/org/apache/iceberg/exceptions/UnprocessableEntityException.java

openinx · 2022-03-21T10:46:42Z

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

+        errorHandler = ErrorHandlers.tableCommitHandler();
+        break;
+
+      case SIMPLE:


What's the usage & meaning of the SIMPLE type ?

A non-transactional commit.

RussellSpitzer · 2022-03-21T15:07:48Z

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

+  @Override
+  public boolean dropTable(TableIdentifier identifier, boolean purge) {
+    String tablePath = tablePath(identifier);
+    // TODO: support purge flagN


typo, also maybe we should throw an error if "UnsupportedOperationException" if purge = true? I guess technically whatever happens is up to the catalog if we don't pass through option, but still maybe best to just throw an error till we have the flag actually propagated?

Purge is true by default, so I think it's best to add it after this goes in. We're planning on adding it as a next step.

RussellSpitzer · 2022-03-21T15:11:36Z

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

+
+  @Override
+  public List<Namespace> listNamespaces(Namespace namespace) throws NoSuchNamespaceException {
+    Preconditions.checkArgument(namespace.isEmpty(), "Cannot list namespaces under parent: %s", namespace);


"Cannot list namespaces with a parent specified: %s passed" ?

The current message has an implication that perhaps another parent would work.

Like purge, this is going to be a quick follow-up. This shouldn't be released so I don't want to spend too much time worrying about it.

RussellSpitzer · 2022-03-21T16:15:54Z

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

+        break;
+
+      case REPLACE:
+        Preconditions.checkState(base != null, "Invalid base metadata: null");


"Invalid base metadata for replace transaction, expected non-null"?

We could potentially change it, but this is fairly consistent with phrasing that is used elsewhere in the code.

In practice, the stack trace that will accompany it as well as the IllegalArgumentException makes it clear in my opinion that null was received and should not have been.

I was matching them message used directly above, that's why I made the suggestion

I included more context above because it isn't clear what was invalid about the metadata. I think the fact that Iceberg expected null is important. Here, the main thing the person reading this needs to know is that metadata was null and that's almost never a good thing.

RussellSpitzer · 2022-03-21T16:18:12Z

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

+        Preconditions.checkState(base == null, "Invalid base metadata for create transaction, expected null: %s", base);
+        requestBuilder = UpdateTableRequest.builderForCreate();
+        baseChanges = createChanges;
+        errorHandler = ErrorHandlers.tableErrorHandler(); // throws NoSuchTableException


Not sure I follow the comment here, shouldn't we through NoSuchTableException in the Replace branch below?

I see it has

https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java#L80

Which we do want

You're right. It should be AlreadyExistsException

RussellSpitzer · 2022-03-21T16:27:50Z

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

+  }
+
+  private static String metadataFileLocation(TableMetadata metadata, String filename) {
+    String metadataLocation = metadata.properties()


nit: maybe rename to customMetadataLocation, otherwise the code below reads a bit like "if metadatalocation is null use metadata.location".

metadata.location() is the table's location, not the metadata location.

RussellSpitzer

Left some minor comments, Looks good to me as a first step

kbendick · 2022-03-21T18:36:32Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+            .addMissing(missing)
+            .addUpdated(updates.keySet())
+            .addRemoved(Sets.difference(removals, missing))
+            .build();


Nit: This line and the ones above it are over indented.

kbendick · 2022-03-21T18:50:44Z

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java

+  }
+
+  private static String tablePath(TableIdentifier ident) {
+    return "v1/namespaces/" + RESTUtil.urlEncode(ident.namespace()) + "/tables/" + ident.name();


What would happen if ident.name() contained a slash? I don't believe that there is anything preventing that from happening at the moment.

I checked and a TableIdentifier can be instantiated with a / in it. I personally think we should disallow this, as it would likely also cause issues with paths on HDFS.

Yeah, this is one of the problems I pointed out when we talked about adding a helper method to generate these paths. This is a test case for that extension.

rdblue · 2022-03-22T22:55:12Z

Thanks for reviewing, @kbendick, @RussellSpitzer, @openinx, and @szehon-ho! I'm merging to unblock the next ones, which are what the remaining comments are about.

I'll also fix a couple of the nits in the next PR, like the indentation in CatalogHandlers and the comment about the error handler.

RussellSpitzer · 2022-03-22T22:56:20Z

SGTM, There is always time for more commits :)

rdblue requested review from RussellSpitzer, danielcweeks, kbendick and szehon-ho March 16, 2022 18:27

rdblue commented Mar 16, 2022

View reviewed changes

github-actions bot added the core label Mar 16, 2022

RussellSpitzer reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Show resolved Hide resolved

RussellSpitzer reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Mar 16, 2022

View reviewed changes

danielcweeks reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java Show resolved Hide resolved

danielcweeks reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java Show resolved Hide resolved

danielcweeks reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseTransaction.java Outdated Show resolved Hide resolved

danielcweeks reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java Outdated Show resolved Hide resolved

danielcweeks reviewed Mar 16, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java Show resolved Hide resolved

github-actions bot added the API label Mar 16, 2022

rdblue added 7 commits March 20, 2022 10:27

Core: Add RESTCatalog and RESTTableOperations.

a64af59

Fix checkstyle.

e876a59

Fix checkstyle.

4372cd0

Fix test checkstyle.

cd998b7

Address review comments.

add07c6

Move property check to UpdateNamespacePropertiesRequest.validate.

0c09032

Remove unused import.

1dcc59b

Fix configuration issue.

6d2faa5

rdblue force-pushed the add-working-rest-catalog branch from 9d58cda to 6d2faa5 Compare March 20, 2022 17:29

rdblue mentioned this pull request Mar 20, 2022

Core: Update DELETE routes to use 204 for success #4366

Merged

kbendick reviewed Mar 21, 2022

View reviewed changes

openinx reviewed Mar 21, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/exceptions/UnprocessableEntityException.java Show resolved Hide resolved

openinx reviewed Mar 21, 2022

View reviewed changes

RussellSpitzer reviewed Mar 21, 2022

View reviewed changes

RussellSpitzer approved these changes Mar 21, 2022

View reviewed changes

kbendick reviewed Mar 21, 2022

View reviewed changes

rdblue merged commit 14d6700 into apache:master Mar 22, 2022

rdblue mentioned this pull request Apr 4, 2022

Core: Add stage-create flag to REST create route #4492

Merged


		Map<String, String> startProperties = catalog.loadNamespaceMetadata(namespace);
		Set<String> missing = Sets.difference(removals, startProperties.keySet());

Core: Add RESTCatalog and RESTTableOperations #4348

Core: Add RESTCatalog and RESTTableOperations #4348

Uh oh!

Conversation

rdblue commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue commented Mar 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Mar 16, 2022 •

edited

Loading

rdblue Mar 16, 2022 •

edited

Loading