Core: Add transaction catalog tests #4319

rdblue · 2022-03-13T22:58:36Z

This extends the recently added CatalogTests class with more tests that cover transactions. There are also minor updates to other classes to make tests pass by standardizing error messages and exceptions.

This implements equals, hashCode, and toString in CharSequenceSet to make testing easier.

This is in support of #4241.

rdblue · 2022-03-13T23:00:34Z

api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java

+
+  @Override
+  public String toString() {
+    return Streams.stream(iterator()).collect(Collectors.joining("CharSequenceSet({", ", ", "})"));


@szehon-ho, I addressed your comment.

nastra

the new tests themselves LGTM, added just a few suggestions

nastra · 2022-03-14T07:54:16Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+
+    TableIdentifier ident = TableIdentifier.of("ns", "table");
+
+    Assert.assertFalse("Table should not exist", catalog.tableExists(ident));


it looks like the first 3 lines are repeated in a lot of tests. would it make sense to extract this into some method?

I think the only thing that could be extracted out would be the create table step. However, some of these tests only create a namespace and not the table. Others create a table in a different namespace, using the added TABLE constant.

We could make a class level constant out of ident, but it might confuse things with TABLE.

Is it possible to use that instead? I'm assuming there's a reason for the change in namespace.

nastra · 2022-03-14T08:04:59Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+  }
+
+  @Test
+  public void testUpdateTableSortOrder() {


testUpdateTableSortOrder and testUpdateTableSortOrderServerSideRetry for example only differ in 1-2 lines, so I woder if we can improve the tests a bit and try and reduce code duplication (in a follow up PR)

szehon-ho · 2022-03-14T19:44:32Z

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

+      if (base != null) {
+        throw new CommitFailedException("Cannot commit: stale table metadata");
+      } else {
+        throw new AlreadyExistsException("Table already exists: %s", tableName());


I didnt look deeply, but wondering why if caller passes base=null and current is not null , we can tell the table already exists? It seems it would be a new assumption to all the calling code?

When base is null, that signals that the commit is trying to create the table and it should not exist. When current is not null, that signals that the table already exists. So in order to throw AlreadyExistsException from the create transaction, this needs to catch that case.

yea makes sense now, thanks. I guess it may be the same concern as @kbendick :) Maybe we should add this to @base argument javadoc in the TableOperations to clarify it, but itd be unrelated.

I added a comment to make this clear.

kbendick · 2022-03-14T19:39:46Z

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

+      if (base != null) {
+        throw new CommitFailedException("Cannot commit: stale table metadata");
+      } else {
+        throw new AlreadyExistsException("Table already exists: %s", tableName());


This exception message reads a little funny to me, but I think in context it will make sense.

For clarification, what's happening here is that we entered some sort of create table transaction, and then at the end of the transaction, the table was created by some other process. Is that correct?

A comment might help make that more clear but I can see how the stack trace will make sense in context.

I added a comment to make this clear.

kbendick · 2022-03-14T19:57:51Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+
+    TableIdentifier ident = TableIdentifier.of("ns", "table");
+
+    Assert.assertFalse("Table should not exist", catalog.tableExists(ident));


I think the only thing that could be extracted out would be the create table step. However, some of these tests only create a namespace and not the table. Others create a table in a different namespace, using the added TABLE constant.

We could make a class level constant out of ident, but it might confuse things with TABLE.

Is it possible to use that instead? I'm assuming there's a reason for the change in namespace.

kbendick · 2022-03-14T19:59:13Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+    Assert.assertTrue("Should be unpartitioned", table.spec().isUnpartitioned());
+    Assert.assertTrue("Should be unsorted", table.sortOrder().isUnsorted());
+    Assert.assertNotNull("Should have table properties", table.properties());
+  }


Do we need to drop the tables that are being created?

No, the catalog is re-initialized every time and data on disk is in a temporary folder.

core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java

rdblue · 2022-03-15T03:19:10Z

Thanks for reviewing, @nastra, @szehon-ho, @kbendick, and @danielcweeks!

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

API: Implement equals and hashCode in CharSequenceSet.

3c43fe7

rdblue force-pushed the add-char-sequence-set-equals branch from 9ed9979 to 3c43fe7 Compare March 13, 2022 23:00

rdblue commented Mar 13, 2022

View reviewed changes

Add more catalog tests, including transactions.

1625c1c

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

rdblue changed the title ~~API: Implement equals and hashCode in CharSequenceSet~~ Core: Add transaction catalog tests Mar 13, 2022

rdblue requested a review from danielcweeks March 13, 2022 23:06

rdblue mentioned this pull request Mar 13, 2022

Core: Add REST catalog table requests and responses #4322

Merged

Fix Hive error message check.

6d4666a

github-actions bot added the hive label Mar 13, 2022

rdblue added 2 commits March 13, 2022 16:34

Remove unnecessary import.

649932a

Fix incorrect Streams class usage.

8f2b1f2

nastra reviewed Mar 14, 2022

View reviewed changes

szehon-ho reviewed Mar 14, 2022

View reviewed changes

kbendick reviewed Mar 14, 2022

View reviewed changes

danielcweeks reviewed Mar 14, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java Show resolved Hide resolved

rdblue added 2 commits March 14, 2022 19:23

Add comment to JDBC error handling.

d57cd11

Add clarification to BaseMetastoreTableOperations.

8bc28b7

danielcweeks approved these changes Mar 15, 2022

View reviewed changes

rdblue merged commit c6a770f into apache:master Mar 15, 2022

rdblue mentioned this pull request Mar 16, 2022

Core: Add RESTCatalog and RESTTableOperations #4348

Merged

rdblue mentioned this pull request May 3, 2022

Core: Add in RenameTableRequest for the RESTCatalog #4448

Merged


		TableIdentifier ident = TableIdentifier.of("ns", "table");

		Assert.assertFalse("Table should not exist", catalog.tableExists(ident));

Core: Add transaction catalog tests #4319

Core: Add transaction catalog tests #4319

Uh oh!

Conversation

rdblue commented Mar 13, 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!

nastra left a comment

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!

szehon-ho Mar 14, 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!

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!

Uh oh!

rdblue commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rdblue commented Mar 13, 2022 •

edited

Loading

szehon-ho Mar 14, 2022 •

edited

Loading