You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2024/02/01 07:36:58 UTC
Re: [PR] IGNITE-20119 Switch index to STOPPING state as a reaction to DROP INDEX [ignite-3]
tkalkirill commented on code in PR #3124:
URL: https://github.com/apache/ignite-3/pull/3124#discussion_r1473808253
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DropIndexCommand.java:
##########
@@ -66,9 +76,14 @@ public List<UpdateEntry> get(Catalog catalog) {
throw new CatalogValidationException("Dropping primary key index is not allowed");
}
- return List.of(
- new DropIndexEntry(index.id(), index.tableId(), schemaName)
- );
+ switch (index.status()) {
+ case AVAILABLE:
+ return List.of(new DropIndexEntry(index.id(), index.tableId()));
+ case STOPPING:
+ throw new CatalogValidationException("Dropping an already dropped index is not allowed");
+ default:
Review Comment:
Perhaps I would make an explicit indication of all statuses, and for `default:` something like “status not supported”.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RemoveIndexCommand.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.commands;
+
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.indexOrThrow;
+import static org.apache.ignite.internal.catalog.descriptors.CatalogIndexStatus.STOPPING;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexStatus;
+import org.apache.ignite.internal.catalog.storage.RemoveIndexEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+
+/**
+ * A command that removes index with specified ID from the Catalog. Only makes sense for an index in the {@link CatalogIndexStatus#STOPPING}
+ * state (so it's about removing a dropped index from the Catalog when we don't need it anymore).
+ *
+ * <p>This is always invoked as a reaction to an internal trigger, not directly by the end user.
+ *
+ * <p>For dropping an index, please refer to {@link DropIndexCommand}.
+ *
+ * @see DropIndexCommand
+ */
+public class RemoveIndexCommand implements CatalogCommand {
+ /** Returns builder to create a command to remove index with specified name. */
+ public static RemoveIndexCommandBuilder builder() {
+ return new Builder();
+ }
+
+ private final int indexId;
+
+ private final String schemaName;
Review Comment:
I think this parameter is not needed.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/RemoveIndexEntry.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.storage;
+
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+
+import java.util.Arrays;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.commands.CatalogUtils;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.events.CatalogEvent;
+import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
+import org.apache.ignite.internal.catalog.events.RemoveIndexEventParameters;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes removal of an index from the Catalog (not the same as dropping it [that just initates the drop sequence]).
+ */
+public class RemoveIndexEntry implements UpdateEntry, Fireable {
+ private static final long serialVersionUID = -6193287919562795933L;
+
+ private final int indexId;
+
+ private final String schemaName;
+
+ /**
+ * Constructs the object.
+ *
+ * @param indexId An id of an index to drop.
+ * @param schemaName Name of the schema to which the index belongs.
+ */
+ public RemoveIndexEntry(int indexId, String schemaName) {
+ this.indexId = indexId;
+ this.schemaName = schemaName;
+ }
+
+ @Override
+ public CatalogEvent eventType() {
+ return CatalogEvent.INDEX_REMOVED;
+ }
+
+ @Override
+ public CatalogEventParameters createEventParameters(long causalityToken, int catalogVersion) {
+ return new RemoveIndexEventParameters(causalityToken, catalogVersion, indexId);
+ }
+
+ @Override
+ public Catalog applyUpdate(Catalog catalog, long causalityToken) {
+ CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
Review Comment:
U can use `org.apache.ignite.internal.catalog.storage.AbstractChangeIndexStatusEntry#schemaByIndexId`
##########
modules/index/src/test/java/org/apache/ignite/internal/index/IndexManagerTest.java:
##########
@@ -415,6 +416,11 @@ private CatalogIndexDescriptor index(int catalogVersion, String indexName) {
return CatalogTestUtils.index(catalogManager, catalogVersion, indexName);
}
+ @Nullable
+ private CatalogIndexDescriptor indexOrNull(int catalogVersion, String indexName) {
+ return CatalogTestUtils.indexOrNull(catalogManager, catalogVersion, indexName);
+ }
Review Comment:
```suggestion
private @Nullable CatalogIndexDescriptor indexOrNull(int catalogVersion, String indexName) {
return CatalogTestUtils.indexOrNull(catalogManager, catalogVersion, indexName);
}
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogIndexStatus.java:
##########
@@ -46,5 +49,14 @@ public enum CatalogIndexStatus {
*
* <p>Readable and writable.</p>
*/
- AVAILABLE
+ AVAILABLE,
+
+ /**
+ * DROP INDEX command has been executed, the index is waiting for RW transactions started when the index was {@link #AVAILABLE}
+ * to finish. After the wait is finished, the index will automatically be removed from the Catalog.
+ *
+ * <p>New RW transactions cannot read the index, but they write to it. RO transactions can still read from it if the readTimestamp
+ * corresponds to a moment when the index was still {@link #AVAILABLE}</p>
Review Comment:
```suggestion
* corresponds to a moment when the index was still {@link #AVAILABLE}.</p>
```
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TableTestUtils.java:
##########
@@ -95,6 +99,23 @@ public static void dropIndex(CatalogManager catalogManager, String schemaName, S
);
}
+ /**
+ * Removes index from the catalog.
+ *
+ * @param catalogManager Catalog manager.
+ * @param indexName Index name.
+ */
+ public static void removeIndex(CatalogManager catalogManager, String indexName) {
+ Integer indexId = getIndexId(catalogManager, indexName, Long.MAX_VALUE);
Review Comment:
U can use `TableTestUtils#getIndexIdStrict`
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropIndexEntry.java:
##########
@@ -17,40 +17,31 @@
package org.apache.ignite.internal.catalog.storage;
-import java.util.Arrays;
-import java.util.Objects;
-import org.apache.ignite.internal.catalog.Catalog;
-import org.apache.ignite.internal.catalog.commands.CatalogUtils;
-import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
-import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexStatus;
import org.apache.ignite.internal.catalog.events.CatalogEvent;
import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
import org.apache.ignite.internal.catalog.events.DropIndexEventParameters;
import org.apache.ignite.internal.tostring.S;
/**
- * Describes deletion of an index.
+ * Describes drop of an index (it's not the final removal of an index from the Catalog, but it's just a switch to
+ * the {@link CatalogIndexStatus#STOPPING} state.
*/
-public class DropIndexEntry implements UpdateEntry, Fireable {
+public class DropIndexEntry extends AbstractChangeIndexStatusEntry implements Fireable {
private static final long serialVersionUID = -604729846502020728L;
- private final int indexId;
-
private final int tableId;
Review Comment:
I think it can be removed.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/RemoveIndexEntry.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.storage;
+
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+
+import java.util.Arrays;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.commands.CatalogUtils;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.events.CatalogEvent;
+import org.apache.ignite.internal.catalog.events.CatalogEventParameters;
+import org.apache.ignite.internal.catalog.events.RemoveIndexEventParameters;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes removal of an index from the Catalog (not the same as dropping it [that just initates the drop sequence]).
+ */
+public class RemoveIndexEntry implements UpdateEntry, Fireable {
+ private static final long serialVersionUID = -6193287919562795933L;
+
+ private final int indexId;
+
+ private final String schemaName;
Review Comment:
I think this parameter is not needed.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RemoveIndexCommandBuilder.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.commands;
+
+import org.apache.ignite.internal.catalog.CatalogCommand;
+
+/** Builder for {@link RemoveIndexCommand}. */
+public interface RemoveIndexCommandBuilder {
+ /** Index ID. */
+ RemoveIndexCommandBuilder indexId(int indexId);
+
+ /** Schema name. */
+ RemoveIndexCommandBuilder schemaName(String schemaName);
Review Comment:
I think this parameter is not needed.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/AbstractChangeIndexStatusEntry.java:
##########
@@ -43,7 +43,7 @@ abstract class AbstractChangeIndexStatusEntry implements UpdateEntry {
}
@Override
- public Catalog applyUpdate(Catalog catalog, long causalityToken) {
+ public final Catalog applyUpdate(Catalog catalog, long causalityToken) {
Review Comment:
Why need 'final'?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildController.java:
##########
@@ -131,12 +131,12 @@ private void addListeners() {
return onIndexBuilding((StartBuildingIndexEventParameters) parameters).thenApply(unused -> false);
});
- catalogService.listen(CatalogEvent.INDEX_DROP, (parameters, exception) -> {
+ catalogService.listen(CatalogEvent.INDEX_REMOVED, (parameters, exception) -> {
Review Comment:
I think that tasks should also be stopped at the `INDEX_STOPPING`.
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildingStarterController.java:
##########
@@ -108,12 +108,12 @@ private void addListeners() {
return onIndexCreate((CreateIndexEventParameters) parameters).thenApply(unused -> false);
});
- catalogService.listen(CatalogEvent.INDEX_DROP, (parameters, exception) -> {
+ catalogService.listen(CatalogEvent.INDEX_REMOVED, (parameters, exception) -> {
Review Comment:
I think that tasks should also be stopped at the `INDEX_STOPPING.`
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -203,6 +201,13 @@ static int getPartitionCountFromCatalog(CatalogService catalogService, int index
return zoneDescriptor.partitions();
}
+ static CatalogIndexDescriptor index(CatalogService catalogService, int indexId, int catalogVersion) {
Review Comment:
missing javadoc
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -131,19 +137,39 @@ public CompletableFuture<Void> start() {
startIndexes();
- catalogService.listen(INDEX_CREATE, (parameters, exception) -> {
+ catalogManager.listen(INDEX_CREATE, (parameters, exception) -> {
if (exception != null) {
return failedFuture(exception);
}
return onIndexCreate((CreateIndexEventParameters) parameters);
});
+ // TODO: IGNITE-21117 - remove this.
+ catalogManager.listen(INDEX_STOPPING, (parameters, exception) -> {
Review Comment:
What about 'INDEX_STOPPING' ?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexAvailabilityController.java:
##########
@@ -236,10 +238,17 @@ private CompletableFuture<?> onIndexBuilding(StartBuildingIndexEventParameters p
});
}
- private CompletableFuture<?> onIndexDrop(DropIndexEventParameters parameters) {
+ private CompletableFuture<?> onIndexRemoved(RemoveIndexEventParameters parameters) {
Review Comment:
I think we need to do the same on `INDEX_STOPPING`.
--
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