j1wonpark commented on code in PR #4362:
URL: https://github.com/apache/amoro/pull/4362#discussion_r3944112648
##########
amoro-ams/src/main/java/org/apache/amoro/server/RestCatalogService.java:
##########
@@ -321,6 +321,7 @@ public void createTable(Context ctx) {
CreateTableRequest request = bodyAsClass(ctx,
CreateTableRequest.class);
request.validate();
String tableName = request.name();
+ checkAlreadyExists(!catalog.tableExists(database, tableName),
"Table", tableName);
Review Comment:
(non-blocking) `InternalCatalogImpl.newTableCreator` already checks
`tableExists` and throws Iceberg's `AlreadyExistsException` before any metadata
is written, for both the stage and non-stage branches. This check and the one
in `commitCreateTable` are redundant — could we drop them and adjust the
description?
##########
amoro-ams/src/test/java/org/apache/amoro/server/TestInternalIcebergCatalogService.java:
##########
@@ -437,6 +438,26 @@ public void testServerCatalogLoadTable() throws
IOException {
Assertions.assertEquals(newRecords.size(), records.size());
}
+ @Test
+ public void testCreateTableAlreadyExists() {
+ nsCatalog.createTable(identifier, schema);
+ Assertions.assertThrows(
+ AlreadyExistsException.class, () ->
nsCatalog.createTable(identifier, schema));
+ }
+
+ @Test
+ public void testCommitCreateTableAlreadyExists(@TempDir Path tempDir) {
+ nsCatalog.createTable(identifier, schema);
+ Path tablePath = tempDir.resolve("staged-table-conflict");
+ Transaction transaction =
+ nsCatalog
+ .buildTable(identifier, schema)
+ .withLocation(tablePath.toUri().toString())
+ .createTransaction();
Review Comment:
(blocking) `createTransaction()` sends the stage-create request eagerly, so
the 409 surfaces on this line, outside the `assertThrows`. More importantly,
both new integration tests pass with `RestCatalogService.java` reverted to
master (this one after reordering it to reach the commit), so they don't
exercise the change. I'd remove both; the `TestRestCatalogService` assertions
are the real regression test (they fail on master with `expected: <Conflict>
but was: <InternalServerError>`).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
