You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2024/03/15 15:42:52 UTC
[PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
rpuch opened a new pull request, #3424:
URL: https://github.com/apache/ignite-3/pull/3424
https://issues.apache.org/jira/browse/IGNITE-21119
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1530352182
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,12 +200,86 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = inFlightFutures.registerFuture(new CompletableFuture<>());
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : "Did not find schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null
+ : "Did not find index " + cmd.indexName() + " in schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ EventListener<CatalogEventParameters> availabilityListener = EventListener.fromConsumer(event -> {
+ if (((MakeIndexAvailableEventParameters) event).indexId() == index.id()) {
+ completeFutureWhenEventVersionActivates(future, event);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_AVAILABLE, availabilityListener);
+
+ EventListener<CatalogEventParameters> removalListener = EventListener.fromConsumer(event -> {
+ if (((RemoveIndexEventParameters) event).indexId() == index.id()) {
+ future.complete(null);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_REMOVED, removalListener);
+
+ // We added listeners, but the index could switch to a state of interest before we added them, so check
+ // explicitly.
+ int latestVersion = catalogManager.latestCatalogVersion();
+ for (int version = creationCatalogVersion + 1; version <= latestVersion; version++) {
Review Comment:
why do we iterate over all future versions? Is it not enough to check the latest one?
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItIndexDdlTest.java:
##########
@@ -98,4 +114,52 @@ private static void tryToCreateIndex(String tableName, String indexName, boolean
private static void tryToDropIndex(String indexName, boolean failIfNotExist) {
sql(String.format("DROP INDEX %s", failIfNotExist ? indexName : "IF EXISTS " + indexName));
}
+
+ private static <T> T preventingIndexBuild(Supplier<T> supplier) {
+ return CLUSTER.aliveNode().transactions().runInTransaction((Function<Transaction, T>) tx -> supplier.get());
+ }
+
+ @Test
+ public void createIndexFutureCompletesWhenIndexBecomesAvailable() {
Review Comment:
it would be nice to have e2e tests ensuring index is available immediately after control is returned from sql api. For such a test it does make sense to return delay duration to its default (1s)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1528004618
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItRwTransactionAndIndexesTest.java:
##########
@@ -62,16 +66,23 @@ void tearDown() {
CLUSTER.runningNodes().forEach(IgniteImpl::stopDroppingMessages);
}
+ @SuppressWarnings("resource")
@Test
- void testCreateIndexInsideRwTransaction() {
+ void testCreateIndexInsideRwTransaction() throws Exception {
+ CatalogManager catalogManager = node().catalogManager();
+
TableImpl table = (TableImpl) createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1, ENGINE_NAME);
- setAwaitIndexAvailability(false);
dropAnyBuildIndexMessages();
Transaction rwTx = beginRwTransaction();
- createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
+
+ assertTrue(waitForCondition(
Review Comment:
But it's the same test as above. If the concern is different, please elaborate.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1531564021
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItIndexDdlTest.java:
##########
@@ -98,4 +114,52 @@ private static void tryToCreateIndex(String tableName, String indexName, boolean
private static void tryToDropIndex(String indexName, boolean failIfNotExist) {
sql(String.format("DROP INDEX %s", failIfNotExist ? indexName : "IF EXISTS " + indexName));
}
+
+ private static <T> T preventingIndexBuild(Supplier<T> supplier) {
+ return CLUSTER.aliveNode().transactions().runInTransaction((Function<Transaction, T>) tx -> supplier.get());
+ }
+
+ @Test
+ public void createIndexFutureCompletesWhenIndexBecomesAvailable() {
Review Comment:
I added another test with production-default cluster configuration (including DD=1s) that uses an index right after the DDL returns.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1528039404
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItRwTransactionAndIndexesTest.java:
##########
@@ -62,16 +66,23 @@ void tearDown() {
CLUSTER.runningNodes().forEach(IgniteImpl::stopDroppingMessages);
}
+ @SuppressWarnings("resource")
@Test
- void testCreateIndexInsideRwTransaction() {
+ void testCreateIndexInsideRwTransaction() throws Exception {
+ CatalogManager catalogManager = node().catalogManager();
+
TableImpl table = (TableImpl) createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1, ENGINE_NAME);
- setAwaitIndexAvailability(false);
dropAnyBuildIndexMessages();
Transaction rwTx = beginRwTransaction();
- createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
+
+ assertTrue(waitForCondition(
Review Comment:
Oh, I see what you mean. Fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1527996494
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -217,6 +217,22 @@ public void testEmptyCatalog() {
assertThat(manager.latestCatalogVersion(), is(0));
}
+ @Test
+ public void assignsSuccessiveCatalogVersions() {
+ CompletableFuture<Integer> version1Future = manager.execute(simpleTable(TABLE_NAME));
+ assertThat(version1Future, willCompleteSuccessfully());
Review Comment:
There is a test that checks explicitly that `execute()` returns successive version numbers. Non of other tests actually needs to know the completion value, it's enough to verify that the future completes with any value. If I add that, this will make the test very rigid for no reason.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1528016649
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManager.java:
##########
@@ -29,16 +29,16 @@ public interface CatalogManager extends IgniteComponent, CatalogService {
* Executes given command.
*
* @param command Command to execute.
- * @return Future representing result of execution.
+ * @return Future representing result of execution (it will be completed with the created catalog version).
*/
- CompletableFuture<Void> execute(CatalogCommand command);
+ CompletableFuture<Integer> execute(CatalogCommand command);
Review Comment:
In `DdlCommandHandler#handleCreateIndex()`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1528018505
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogIndexStatus.java:
##########
@@ -80,6 +80,19 @@ public boolean isAlive() {
return this != STOPPING;
}
+ /**
+ * Returns {@code true} if the index is {@link #AVAILABLE} or follows it.
+ */
+ public boolean isAvailableOrLater() {
Review Comment:
It's about the index status, so it's naturally to have it on it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1530540385
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,12 +200,86 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = inFlightFutures.registerFuture(new CompletableFuture<>());
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : "Did not find schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null
+ : "Did not find index " + cmd.indexName() + " in schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ EventListener<CatalogEventParameters> availabilityListener = EventListener.fromConsumer(event -> {
+ if (((MakeIndexAvailableEventParameters) event).indexId() == index.id()) {
+ completeFutureWhenEventVersionActivates(future, event);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_AVAILABLE, availabilityListener);
+
+ EventListener<CatalogEventParameters> removalListener = EventListener.fromConsumer(event -> {
+ if (((RemoveIndexEventParameters) event).indexId() == index.id()) {
+ future.complete(null);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_REMOVED, removalListener);
+
+ // We added listeners, but the index could switch to a state of interest before we added them, so check
+ // explicitly.
+ int latestVersion = catalogManager.latestCatalogVersion();
+ for (int version = creationCatalogVersion + 1; version <= latestVersion; version++) {
Review Comment:
If we missed a few catalog updates, this allows us to find the earliest of them in which the index has become AVAILABLE, so the user will need to wait a little less (because we don't just complete the future as soon as we see an AVAILABLE version in the Catalog, we take its activation ts and wait till it comes). Also, it's unlikely that a thousand catalog versions will appear for scanning while we are adding the listeners :)
To sum up: the scanning might yield an earlier completion moment, but if we take the latest version, it won't be too different, so I'll change this if you insist.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1530561367
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,12 +200,86 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> inBusyLock(busyLock, () -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion)))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = inFlightFutures.registerFuture(new CompletableFuture<>());
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : "Did not find schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null
+ : "Did not find index " + cmd.indexName() + " in schema " + cmd.schemaName() + " in version " + creationCatalogVersion;
+
+ EventListener<CatalogEventParameters> availabilityListener = EventListener.fromConsumer(event -> {
+ if (((MakeIndexAvailableEventParameters) event).indexId() == index.id()) {
+ completeFutureWhenEventVersionActivates(future, event);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_AVAILABLE, availabilityListener);
+
+ EventListener<CatalogEventParameters> removalListener = EventListener.fromConsumer(event -> {
+ if (((RemoveIndexEventParameters) event).indexId() == index.id()) {
+ future.complete(null);
+ }
+ });
+ catalogManager.listen(CatalogEvent.INDEX_REMOVED, removalListener);
+
+ // We added listeners, but the index could switch to a state of interest before we added them, so check
+ // explicitly.
+ int latestVersion = catalogManager.latestCatalogVersion();
+ for (int version = creationCatalogVersion + 1; version <= latestVersion; version++) {
Review Comment:
> the scanning might yield an earlier completion moment
I missed this part, thanks for explanation! I'm ok with current approach
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1528002831
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItRwTransactionAndIndexesTest.java:
##########
@@ -62,16 +66,23 @@ void tearDown() {
CLUSTER.runningNodes().forEach(IgniteImpl::stopDroppingMessages);
}
+ @SuppressWarnings("resource")
@Test
- void testCreateIndexInsideRwTransaction() {
+ void testCreateIndexInsideRwTransaction() throws Exception {
+ CatalogManager catalogManager = node().catalogManager();
+
TableImpl table = (TableImpl) createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1, ENGINE_NAME);
- setAwaitIndexAvailability(false);
dropAnyBuildIndexMessages();
Transaction rwTx = beginRwTransaction();
- createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
Review Comment:
This test is built in such a way that the iindex never becomes AVAILABLE
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21119 Complete index creation future when the index is AVAILABLE or removed [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3424:
URL: https://github.com/apache/ignite-3/pull/3424#discussion_r1527924003
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexOneNodeTest.java:
##########
@@ -93,7 +87,7 @@ void testRecoverBuildingIndex() throws Exception {
return false;
});
- createIndex(TABLE_NAME, INDEX_NAME, "ID");
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, "ID"));
Review Comment:
Maybe it would be better to wait for the futures to finish executing?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexOneNodeTest.java:
##########
@@ -285,13 +279,10 @@ void testBuildingIndexWithUpdateSchemaAfterCreateIndex() throws Exception {
Transaction rwTx = node().transactions().begin(new TransactionOptions().readOnly(false));
try {
- setAwaitIndexAvailability(false);
-
- createIndex(TABLE_NAME, INDEX_NAME, columName);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, columName));
Review Comment:
Same
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItDropIndexMultipleNodesTest.java:
##########
@@ -264,7 +269,7 @@ void testDropIndexDuringBuilding() {
CompletableFuture<Void> indexRemovedFuture = indexRemovedFuture();
- createIndex();
+ runAsync(() -> createIndex());
Review Comment:
Same
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItDropIndexMultipleNodesTest.java:
##########
@@ -239,7 +242,12 @@ void testDropIndexAfterRegistering() {
runInRwTransaction(CLUSTER.aliveNode(), tx -> {
// Create an index inside a transaction, this will prevent the index from building.
try {
- createIndex();
+ runAsync(() -> createIndex());
Review Comment:
Same about wait future.
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -217,6 +217,22 @@ public void testEmptyCatalog() {
assertThat(manager.latestCatalogVersion(), is(0));
}
+ @Test
+ public void assignsSuccessiveCatalogVersions() {
+ CompletableFuture<Integer> version1Future = manager.execute(simpleTable(TABLE_NAME));
+ assertThat(version1Future, willCompleteSuccessfully());
Review Comment:
Can you do `willBe(1)` and etc?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItRwTransactionAndIndexesTest.java:
##########
@@ -62,16 +66,23 @@ void tearDown() {
CLUSTER.runningNodes().forEach(IgniteImpl::stopDroppingMessages);
}
+ @SuppressWarnings("resource")
@Test
- void testCreateIndexInsideRwTransaction() {
+ void testCreateIndexInsideRwTransaction() throws Exception {
+ CatalogManager catalogManager = node().catalogManager();
+
TableImpl table = (TableImpl) createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1, ENGINE_NAME);
- setAwaitIndexAvailability(false);
dropAnyBuildIndexMessages();
Transaction rwTx = beginRwTransaction();
- createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
Review Comment:
Same
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogIndexStatus.java:
##########
@@ -80,6 +80,19 @@ public boolean isAlive() {
return this != STOPPING;
}
+ /**
+ * Returns {@code true} if the index is {@link #AVAILABLE} or follows it.
+ */
+ public boolean isAvailableOrLater() {
Review Comment:
Maybe move to `CatalogUtils` ?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItDropIndexOneNodeTest.java:
##########
@@ -65,14 +69,23 @@ void testCreateIndexAfterDrop() {
@Test
void testCreateIndexAfterDropWhileTransactionInProgress() {
+ CatalogManager catalogManager = CLUSTER.aliveNode().catalogManager();
+
runInRwTransaction(tx -> {
dropIndex(INDEX_NAME);
- // The new index will not become available, since we are inside a transaction that has been started before this index was
- // created.
- setAwaitIndexAvailability(false);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
Review Comment:
same
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,9 +191,74 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = new CompletableFuture<>();
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : cmd.schemaName();
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null : cmd.indexName();
+
+ EventListener<CatalogEventParameters> availabilityListener = event -> {
+ if (((MakeIndexAvailableEventParameters) event).indexId() == index.id()) {
Review Comment:
U can use `EventListener#fromConsumer`
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItIndexDdlTest.java:
##########
@@ -83,10 +95,12 @@ void testDropIndex() {
* @param failIfExist Throw an exception if the index exist.
*/
private static void tryToCreateIndex(String tableName, String indexName, boolean failIfExist) {
+ Loggers.forClass(ItIndexDdlTest.class).info("XXX starting index creation");
Review Comment:
Let's move the logger to the class and not use "XXX".
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItDataSchemaSyncTest.java:
##########
@@ -198,18 +198,14 @@ public void queryWaitAppropriateSchema() {
createTable(ignite0, TABLE_NAME);
- WatchListenerInhibitor listenerInhibitor = WatchListenerInhibitor.metastorageEventsInhibitor(ignite1);
-
- listenerInhibitor.startInhibit();
+ WatchListenerInhibitor.metastorageEventsInhibitor(ignite1).withInhibition(() -> {
+ runAsync(() -> sql(ignite0, "CREATE INDEX idx1 ON " + TABLE_NAME + "(valint)"));
Review Comment:
same
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,9 +191,74 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = new CompletableFuture<>();
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : cmd.schemaName();
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null : cmd.indexName();
+
+ EventListener<CatalogEventParameters> availabilityListener = event -> {
+ if (((MakeIndexAvailableEventParameters) event).indexId() == index.id()) {
+ completeFutureWhenEventVersionActivates(future, event);
+ }
+
+ return falseCompletedFuture();
+ };
+ catalogManager.listen(CatalogEvent.INDEX_AVAILABLE, availabilityListener);
+
+ EventListener<CatalogEventParameters> removalListener = event -> {
+ if (((RemoveIndexEventParameters) event).indexId() == index.id()) {
Review Comment:
Same about fromComsumer
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,9 +191,74 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = new CompletableFuture<>();
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : cmd.schemaName();
Review Comment:
I think it would be good to indicate the catalog version in the error message.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandler.java:
##########
@@ -169,9 +191,74 @@ private static BiFunction<Object, Throwable, Boolean> handleModificationResult(b
/** Handles create index command. */
private CompletableFuture<Boolean> handleCreateIndex(CreateIndexCommand cmd) {
return catalogManager.execute(DdlToCatalogCommandConverter.convert(cmd))
+ .thenCompose(catalogVersion -> waitTillIndexBecomesAvailableOrRemoved(cmd, catalogVersion))
.handle(handleModificationResult(cmd.ifNotExists(), IndexExistsValidationException.class));
}
+ private CompletionStage<Void> waitTillIndexBecomesAvailableOrRemoved(CreateIndexCommand cmd, Integer creationCatalogVersion) {
+ CompletableFuture<Void> future = new CompletableFuture<>();
+
+ Catalog catalog = catalogManager.catalog(creationCatalogVersion);
+ assert catalog != null : creationCatalogVersion;
+
+ CatalogSchemaDescriptor schema = catalog.schema(cmd.schemaName());
+ assert schema != null : cmd.schemaName();
+
+ CatalogIndexDescriptor index = schema.aliveIndex(cmd.indexName());
+ assert index != null : cmd.indexName();
Review Comment:
same
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItDropIndexOneNodeTest.java:
##########
@@ -65,14 +69,23 @@ void testCreateIndexAfterDrop() {
@Test
void testCreateIndexAfterDropWhileTransactionInProgress() {
+ CatalogManager catalogManager = CLUSTER.aliveNode().catalogManager();
+
runInRwTransaction(tx -> {
dropIndex(INDEX_NAME);
- // The new index will not become available, since we are inside a transaction that has been started before this index was
- // created.
- setAwaitIndexAvailability(false);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
+
+ try {
+ assertTrue(waitForCondition(
Review Comment:
Let's add it as a helper method to the test code?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItTablesApiTest.java:
##########
@@ -256,6 +263,13 @@ public void testAddIndex() {
tryToCreateIndex(ignite0, TABLE_NAME, false);
}
+ private static void waitForIndexToAppearInAnyState(IgniteImpl ignite0) throws InterruptedException {
Review Comment:
same
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItRwTransactionAndIndexesTest.java:
##########
@@ -62,16 +66,23 @@ void tearDown() {
CLUSTER.runningNodes().forEach(IgniteImpl::stopDroppingMessages);
}
+ @SuppressWarnings("resource")
@Test
- void testCreateIndexInsideRwTransaction() {
+ void testCreateIndexInsideRwTransaction() throws Exception {
+ CatalogManager catalogManager = node().catalogManager();
+
TableImpl table = (TableImpl) createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1, ENGINE_NAME);
- setAwaitIndexAvailability(false);
dropAnyBuildIndexMessages();
Transaction rwTx = beginRwTransaction();
- createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME);
+ runAsync(() -> createIndex(TABLE_NAME, INDEX_NAME, COLUMN_NAME));
+
+ assertTrue(waitForCondition(
Review Comment:
Same
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManager.java:
##########
@@ -29,16 +29,16 @@ public interface CatalogManager extends IgniteComponent, CatalogService {
* Executes given command.
*
* @param command Command to execute.
- * @return Future representing result of execution.
+ * @return Future representing result of execution (it will be completed with the created catalog version).
*/
- CompletableFuture<Void> execute(CatalogCommand command);
+ CompletableFuture<Integer> execute(CatalogCommand command);
Review Comment:
Didn’t notice where you are using the result of the future, not in the test code?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org