korlov42 commented on code in PR #3221:
URL: https://github.com/apache/ignite-3/pull/3221#discussion_r1493784350
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -498,6 +539,10 @@ private CompletableFuture<Void> handle(VersionedUpdate
update, HybridTimestamp m
}
}
+ private static CatalogZoneDescriptor tableZoneDescriptor(Catalog
oldCatalog, int tableId) {
Review Comment:
does prefix `old` make sense here? (`oldCatalog`)
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -498,6 +539,10 @@ private CompletableFuture<Void> handle(VersionedUpdate
update, HybridTimestamp m
}
}
+ private static CatalogZoneDescriptor tableZoneDescriptor(Catalog
oldCatalog, int tableId) {
+ return
Objects.requireNonNull(oldCatalog.zone(Objects.requireNonNull(oldCatalog.table(tableId)).zoneId()));
Review Comment:
such pattern doesn't make any sense. You either have to provide context for
every `requireNonNull` (like `Objects.requireNonNull(oldCatalog.table(tableId),
"table")`), or put `requireNonNull` on distinct line with temporal variable
(`var table = Objects.requireNonNull(oldCatalog.table(tableId))`). Otherwise
you will get `NPE at line X` with no chance to get what exactly was not found
in catalog
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -393,6 +395,31 @@ public void testDropTable() {
assertSame(schema, manager.activeSchema(clock.nowLong()));
}
+ @Test
+ void testReCreateTableWithSameName() {
+ assertThat(manager.execute(simpleTable(TABLE_NAME)),
willBe(nullValue()));
+
+ int catalogVersion = manager.latestCatalogVersion();
+ CatalogTableDescriptor table1 = manager.table(TABLE_NAME,
clock.nowLong());
+ assertNotNull(table1);
+
+ // Drop table.
+ assertThat(manager.execute(dropTableCommand(TABLE_NAME)),
willBe(nullValue()));
+ assertNull(manager.table(TABLE_NAME, clock.nowLong()));
+
+ // Re-create table with same name.
+ assertThat(manager.execute(simpleTable(TABLE_NAME)),
willBe(nullValue()));
+
+ CatalogTableDescriptor table2 = manager.table(TABLE_NAME,
clock.nowLong());
+ assertNotNull(table2);
+
+ // Ensure these are different tables.
+ assertNotEquals(table1.id(), table2.id());
+
+ // Ensure table is available for historical queries.
+ manager.table(table1.id(), catalogVersion);
Review Comment:
should we add any assertion here?
--
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]
