rpuch commented on code in PR #3448: URL: https://github.com/apache/ignite-3/pull/3448#discussion_r1538653352
########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogExecutionResult.java: ########## @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog; + +import java.util.Objects; + +/** Holder for catalog command execution result. */ +public final class CatalogExecutionResult { + private final int version; + private final Boolean applied; Review Comment: Can it be `null`? If not, then probably `boolean` is better; if yes, then please annotate it as `@Nullable` and explain in the javadoc what `null` means ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -377,16 +377,16 @@ private void truncateUpTo(Catalog catalog) { LOG.info("Catalog history was truncated up to version=" + catalog.version()); } - private CompletableFuture<Integer> saveUpdateAndWaitForActivation(UpdateProducer updateProducer) { + private CompletableFuture<CatalogExecutionResult> saveUpdateAndWaitForActivation(UpdateProducer updateProducer) { return saveUpdate(updateProducer, 0) - .thenCompose(newVersion -> { - Catalog catalog = catalogByVer.get(newVersion); + .thenCompose(state -> { Review Comment: ```suggestion .thenCompose(executionResult -> { ``` ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java: ########## @@ -101,19 +101,28 @@ public static void validateZoneFilter(@Nullable String filter) { * * @param schema Schema to look up relation with specified name. * @param name Name of the relation to look up. + * @param checkType Type this check is applicable to. * @throws CatalogValidationException If relation with specified name exists in given schema. */ - public static void ensureNoTableIndexOrSysViewExistsWithGivenName(CatalogSchemaDescriptor schema, String name) { - if (schema.aliveIndex(name) != null) { + public static void ensureNoTableIndexOrSysViewExistsWithGivenName(CatalogSchemaDescriptor schema, String name, int checkType) { + if ((checkType & CheckInstanceType.INDEX) != 0 && schema.aliveIndex(name) != null) { throw new IndexExistsValidationException(format("Index with name '{}.{}' already exists", schema.name(), name)); } - if (schema.table(name) != null) { + if ((checkType & CheckInstanceType.TABLE) != 0 && schema.table(name) != null) { throw new TableExistsValidationException(format("Table with name '{}.{}' already exists", schema.name(), name)); } - if (schema.systemView(name) != null) { + if ((checkType & CheckInstanceType.SYS_VIEW) != 0 && schema.systemView(name) != null) { throw new CatalogValidationException(format("System view with name '{}.{}' already exists", schema.name(), name)); } } + + /** Types corresponds to appropriate check. */ + public interface CheckInstanceType { + int TABLE = 1; Review Comment: How about making `CheckInstanceType` an enum and using a `Set<CheckInstanceType>` (represented with `EnumSet`) instead of a bitmask? ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java: ########## @@ -102,7 +110,14 @@ private CreateTableCommand( public List<UpdateEntry> get(Catalog catalog) { CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName); - ensureNoTableIndexOrSysViewExistsWithGivenName(schema, tableName); + int checkType = ignoreErrorIfExist ? SYS_VIEW | INDEX : ALL; + + ensureNoTableIndexOrSysViewExistsWithGivenName(schema, tableName, checkType); + + // table already created, but probably current schema still not active Review Comment: ```suggestion // Table already created by someone else. ``` ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandBuilder.java: ########## @@ -44,4 +44,7 @@ public interface CreateTableCommandBuilder extends AbstractTableCommandBuilder<C /** A name of the zone to create new table in. Should not be null or blank. */ CreateTableCommandBuilder zone(@Nullable String zoneName); + + /** Ignore error if table with given name already exists. */ + CreateTableCommandBuilder ignoreErrorIfTableExist(boolean ignore); Review Comment: ```suggestion CreateTableCommandBuilder ignoreErrorIfTableExists(boolean ignore); ``` ########## modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java: ########## @@ -1162,6 +1163,58 @@ public void catalogActivationTime() throws Exception { } } + @Test + public void sequentialCreateTableCall() throws Exception { + long delayDuration = TimeUnit.DAYS.toMillis(365); + + int partitionIdleSafeTimePropagationPeriod = 0; + + CatalogManagerImpl manager = new CatalogManagerImpl(updateLog, clockWaiter, clock, delayDuration, + partitionIdleSafeTimePropagationPeriod); + + manager.start(); + + try { + CatalogCommand createTableCommand = simpleTable(TABLE_NAME, true); Review Comment: ```suggestion CatalogCommand createTableCommand = spy(simpleTable(TABLE_NAME, true)); ``` ########## modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java: ########## @@ -1162,6 +1163,58 @@ public void catalogActivationTime() throws Exception { } } + @Test + public void sequentialCreateTableCall() throws Exception { + long delayDuration = TimeUnit.DAYS.toMillis(365); + + int partitionIdleSafeTimePropagationPeriod = 0; + + CatalogManagerImpl manager = new CatalogManagerImpl(updateLog, clockWaiter, clock, delayDuration, + partitionIdleSafeTimePropagationPeriod); + + manager.start(); + + try { + CatalogCommand createTableCommand = simpleTable(TABLE_NAME, true); + + createTableCommand = Mockito.spy(createTableCommand); Review Comment: Please import `spy()` statically (it's designed to be used in this way) ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java: ########## @@ -102,7 +110,14 @@ private CreateTableCommand( public List<UpdateEntry> get(Catalog catalog) { CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName); - ensureNoTableIndexOrSysViewExistsWithGivenName(schema, tableName); + int checkType = ignoreErrorIfExist ? SYS_VIEW | INDEX : ALL; + + ensureNoTableIndexOrSysViewExistsWithGivenName(schema, tableName, checkType); + + // table already created, but probably current schema still not active + if (ignoreErrorIfExist && schema.table(tableName) != null) { + return List.of(); Review Comment: I'm still not sure it's best to always equalize 'no entries' with 'do not create because it already exists AND we were asked to be quiet because of IF NOT EXISTS'. Is an explicit flag for this ('did not create anything as it's already created') in the return value an option? ########## modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java: ########## @@ -1162,6 +1163,58 @@ public void catalogActivationTime() throws Exception { } } + @Test + public void sequentialCreateTableCall() throws Exception { Review Comment: ```suggestion public void createTableIfNotExistWaitsActivationEvenIfTableExists() throws Exception { ``` ########## modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/BaseCatalogManagerTest.java: ########## @@ -89,6 +89,7 @@ void setUp() { metastore = StandaloneMetaStorageManager.create(new SimpleInMemoryKeyValueStorage(NODE_NAME)); updateLog = spy(new UpdateLogImpl(metastore)); + clock = new HybridClockImpl(); Review Comment: Why is clock instance recreated? -- 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]
