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 2023/11/24 05:29:45 UTC
[PR] IGNITE-20884 Registering all indexes for active tables on node recovery [ignite-3]
tkalkirill opened a new pull request, #2871:
URL: https://github.com/apache/ignite-3/pull/2871
https://issues.apache.org/jira/browse/IGNITE-20884
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405968515
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
Review Comment:
Yes, I can use it, but I think my current approach will be faster.
Filling map: v - count of versions, n - count of indexes
my -> O(v*n)
our -> O(v*n*logN)
Result: n - count of indexes
my -> O(n*logN)
your -> O(n)
I think in general i can remove sorting and simply return a collection of indexes.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406046761
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
Review Comment:
I'm not talking about efficiency (it's nearly the same), it would be simply shorter in terms of code. If you can remove the sorting, then my suggestion is irrelevant.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406012589
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1087,14 +1096,17 @@ private CompletableFuture<?> createTableLocally(long causalityToken, int catalog
* @param zoneDescriptor Catalog distributed zone descriptor.
* @param assignmentsFuture Future with assignments.
* @param catalogVersion Catalog version on which the table was created.
+ * @param onNodeRecovery On node recovery.
Review Comment:
`Flag of which determines whether this method is called when the node is restored or during its standard operation.`
Good?
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406002728
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2085,9 +2097,11 @@ private CompletableFuture<Void> destroyPartitionStorages(TablePartitionId tableP
);
}
- private int[] collectTableIndexIds(int tableId, int catalogVersion) {
- return catalogService.indexes(catalogVersion, tableId).stream()
- .mapToInt(CatalogIndexDescriptor::id)
+ private int[] collectTableIndexIds(int tableId, int catalogVersion, boolean onNodeRecovery) {
+ int catalogVersionFrom = onNodeRecovery ? catalogService.latestCatalogVersion() : catalogVersion;
Review Comment:
I have a mistake here, thanks for noticing.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405993723
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
Review Comment:
Because under the hood it uses a `join` on the same future and can freeze the test.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406045562
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
Review Comment:
Can you please rephrase the javadoc?
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405990127
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
Review Comment:
I just did it like in the neighboring integration tests without really thinking, do you think it’s worth changing the extension?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
Review Comment:
I just did it like in the neighboring integration tests without really thinking, do you think it’s worth changing the extension?
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406050770
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
Review Comment:
I don't know, if you can't find a better place, then let's leave it 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406142376
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1087,14 +1096,17 @@ private CompletableFuture<?> createTableLocally(long causalityToken, int catalog
* @param zoneDescriptor Catalog distributed zone descriptor.
* @param assignmentsFuture Future with assignments.
* @param catalogVersion Catalog version on which the table was created.
+ * @param onNodeRecovery On node recovery.
Review Comment:
fix it
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1044,9 +1045,16 @@ private void cleanUpTablesResources(Map<Integer, TableImpl> tables) {
* @param causalityToken Causality token.
* @param catalogVersion Catalog version on which the table was created.
* @param tableDescriptor Catalog table descriptor.
+ * @param onNodeRecovery On node recovery.
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406156335
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
Review Comment:
Left a comment
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405984892
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CatalogUtilsTest.java:
##########
@@ -189,6 +217,187 @@ void testFromParamsAndPreviousValueWithAutoAdjustFields() {
);
}
+ @Test
+ void testCollectIndexesAfterCreateTable() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, INDEX_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndexAndMakeAvailableIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
Review Comment:
Checks that the method will return both available and registered indexes.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405980722
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
Review Comment:
No, where do you suggest moving it? **TableManager** is already very overloaded IMHO.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405981944
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
+
+ for (int catalogVersion = catalogVersionFrom; catalogVersion <= catalogVersionTo; catalogVersion++) {
+ for (CatalogIndexDescriptor index : catalogService.indexes(catalogVersion, tableId)) {
+ indexByIdMap.put(index.id(), index);
+ }
+ }
+
+ assert !indexByIdMap.isEmpty()
+ : String.format("catalogVersionFrom=%s, catalogVersionTo=%s, tableId=%s", catalogVersionFrom, catalogVersionTo, tableId);
+
+ var res = new ArrayList<>(indexByIdMap.values());
+
+ res.sort(comparingInt(CatalogObjectDescriptor::id));
+
+ return unmodifiableList(res);
Review Comment:
You're right, just don't mess around with this list. Maybe I overdid it, delete 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406047954
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
Review Comment:
Yes, it's clearly not an "SQL" test)
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
Review Comment:
that sucks
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405726146
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
Review Comment:
What do you mean here? That catalog should contain at least one index of the given table?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
Review Comment:
```suggestion
* Collects all table indexes (including dropped) that the table has in the requested catalog version range.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
Review Comment:
Can we use `Int2ObjectRBTreeMap` here? This way your descriptors will be sorted by ID
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
+
+ for (int catalogVersion = catalogVersionFrom; catalogVersion <= catalogVersionTo; catalogVersion++) {
+ for (CatalogIndexDescriptor index : catalogService.indexes(catalogVersion, tableId)) {
+ indexByIdMap.put(index.id(), index);
+ }
+ }
+
+ assert !indexByIdMap.isEmpty()
+ : String.format("catalogVersionFrom=%s, catalogVersionTo=%s, tableId=%s", catalogVersionFrom, catalogVersionTo, tableId);
+
+ var res = new ArrayList<>(indexByIdMap.values());
+
+ res.sort(comparingInt(CatalogObjectDescriptor::id));
+
+ return unmodifiableList(res);
Review Comment:
Why do you need an `unmodifiableList` here? It's a local variable anyway
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
Review Comment:
Why does this class extend `BaseSqlIntegrationTest` and not `ClusterPerTestIntegrationTest`?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
Review Comment:
Why not use the sync version of the same method?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1044,9 +1045,16 @@ private void cleanUpTablesResources(Map<Integer, TableImpl> tables) {
* @param causalityToken Causality token.
* @param catalogVersion Catalog version on which the table was created.
* @param tableDescriptor Catalog table descriptor.
+ * @param onNodeRecovery On node recovery.
Review Comment:
This is not a good description
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2085,9 +2097,11 @@ private CompletableFuture<Void> destroyPartitionStorages(TablePartitionId tableP
);
}
- private int[] collectTableIndexIds(int tableId, int catalogVersion) {
- return catalogService.indexes(catalogVersion, tableId).stream()
- .mapToInt(CatalogIndexDescriptor::id)
+ private int[] collectTableIndexIds(int tableId, int catalogVersion, boolean onNodeRecovery) {
+ int catalogVersionFrom = onNodeRecovery ? catalogService.latestCatalogVersion() : catalogVersion;
Review Comment:
Can you please add a comment about why do we use the latest catalog version when a node is starting?
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CatalogUtilsTest.java:
##########
@@ -189,6 +217,187 @@ void testFromParamsAndPreviousValueWithAutoAdjustFields() {
);
}
+ @Test
+ void testCollectIndexesAfterCreateTable() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, INDEX_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndexAndMakeAvailableIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
Review Comment:
What scenario does this code test? The test above already checks that the index is available after it's been created
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
Review Comment:
I can see that this methods is only used in one place, do we really need a utility method for that?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
Review Comment:
There's a `Cluster#restartNode` method
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -444,4 +445,38 @@ private static <T> BiFunction<T, Throwable, CompletableFuture<T>> updater(Functi
return updateFunction.apply(t);
};
}
+
+ /**
+ * Collects indexes (including deleted ones) for tables (tables from the latest version of the catalog) from the earliest to the latest
+ * version of the catalog that need to be started on node recovery.
+ *
+ * @param catalogService Catalog service.
+ */
+ static Map<CatalogTableDescriptor, Collection<CatalogIndexDescriptor>> collectIndexesForRecovery(CatalogService catalogService) {
+ int earliestCatalogVersion = catalogService.earliestCatalogVersion();
+ int latestCatalogVersion = catalogService.latestCatalogVersion();
+
+ var indexesByTableId = new Int2ObjectOpenHashMap<Int2ObjectMap<CatalogIndexDescriptor>>();
+
+ for (CatalogTableDescriptor table : catalogService.tables(latestCatalogVersion)) {
+ indexesByTableId.computeIfAbsent(table.id(), indexById -> new Int2ObjectOpenHashMap<>());
Review Comment:
Is it possible to get duplicate table IDs here? Why not use a simple `put`?
##########
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##########
@@ -298,4 +301,40 @@ public void stop() throws Exception {
}
}
+
+ /**
+ * Searches for a table by name in the requested version of the catalog.
+ *
+ * @param catalogService Catalog service.
+ * @param catalogVersion Catalog version in which to find the table.
+ * @param tableName Table name.
+ */
+ public static CatalogTableDescriptor table(CatalogService catalogService, int catalogVersion, String tableName) {
+ CatalogTableDescriptor tableDescriptor = catalogService.tables(catalogVersion).stream()
+ .filter(table -> tableName.equals(table.name()))
+ .findFirst()
+ .orElse(null);
+
+ assertNotNull(tableDescriptor, "catalogVersion=" + catalogVersion + ", tableName=" + tableName);
+
+ return tableDescriptor;
+ }
+
+ /**
+ * Searches for a index by name in the requested version of the catalog.
Review Comment:
```suggestion
* Searches for an index by name in the requested version of the catalog.
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1087,14 +1096,17 @@ private CompletableFuture<?> createTableLocally(long causalityToken, int catalog
* @param zoneDescriptor Catalog distributed zone descriptor.
* @param assignmentsFuture Future with assignments.
* @param catalogVersion Catalog version on which the table was created.
+ * @param onNodeRecovery On node recovery.
Review Comment:
This is not a good description
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
+
+ assertThat(tableFuture, willCompleteSuccessfully());
+
+ return (TableImpl) tableFuture.join();
+ }
+
+ private static List<Integer> collectIndexIdsFromTable(TableImpl table, int partitionId) {
+ CompletableFuture<List<Integer>> future = runAsync(() -> table.indexStorageAdapters(partitionId).get())
Review Comment:
why do you call `runAsync` here if you then wait for the future synchronously?
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CatalogUtilsTest.java:
##########
@@ -189,6 +217,187 @@ void testFromParamsAndPreviousValueWithAutoAdjustFields() {
);
}
+ @Test
+ void testCollectIndexesAfterCreateTable() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, INDEX_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndexAndMakeAvailableIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, indexName0),
+ index(catalogManager, latestCatalogVersion, indexName1)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, indexName0),
+ index(catalogManager, latestCatalogVersion, indexName1)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterDropIndexes() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
+
+ int catalogVersionBeforeDropIndex0 = catalogManager.latestCatalogVersion();
+
+ dropIndex(catalogManager, indexName0);
+
+ int catalogVersionBeforeDropIndex1 = catalogManager.latestCatalogVersion();
+
+ dropIndex(catalogManager, indexName1);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, catalogVersionBeforeDropIndex0, indexName0),
+ index(catalogManager, catalogVersionBeforeDropIndex1, indexName1)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesComplexCase() throws Exception {
Review Comment:
What do you mean by "complex case"? Please leave some comments
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405990931
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406147440
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
+
+ assertThat(tableFuture, willCompleteSuccessfully());
+
+ return (TableImpl) tableFuture.join();
+ }
+
+ private static List<Integer> collectIndexIdsFromTable(TableImpl table, int partitionId) {
+ CompletableFuture<List<Integer>> future = runAsync(() -> table.indexStorageAdapters(partitionId).get())
Review Comment:
did 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405968515
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
Review Comment:
Yes, I can use it, but I think my current approach will be faster.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406049110
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
+
+ assertThat(tableFuture, willCompleteSuccessfully());
+
+ return (TableImpl) tableFuture.join();
+ }
+
+ private static List<Integer> collectIndexIdsFromTable(TableImpl table, int partitionId) {
+ CompletableFuture<List<Integer>> future = runAsync(() -> table.indexStorageAdapters(partitionId).get())
Review Comment:
please leave a comment
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406173368
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CatalogUtilsTest.java:
##########
@@ -189,6 +217,187 @@ void testFromParamsAndPreviousValueWithAutoAdjustFields() {
);
}
+ @Test
+ void testCollectIndexesAfterCreateTable() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, INDEX_NAME);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, INDEX_NAME)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterCreateIndexAndMakeAvailableIndex() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, indexName0),
+ index(catalogManager, latestCatalogVersion, indexName1)
+ )
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, latestCatalogVersion, indexName0),
+ index(catalogManager, latestCatalogVersion, indexName1)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesAfterDropIndexes() throws Exception {
+ withCatalogManager(catalogManager -> {
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createTable(catalogManager, TABLE_NAME);
+ createIndex(catalogManager, TABLE_NAME, indexName0);
+ createIndex(catalogManager, TABLE_NAME, indexName1);
+
+ makeIndexAvailable(catalogManager, indexName1);
+
+ int catalogVersionBeforeDropIndex0 = catalogManager.latestCatalogVersion();
+
+ dropIndex(catalogManager, indexName0);
+
+ int catalogVersionBeforeDropIndex1 = catalogManager.latestCatalogVersion();
+
+ dropIndex(catalogManager, indexName1);
+
+ int latestCatalogVersion = catalogManager.latestCatalogVersion();
+ int earliestCatalogVersion = catalogManager.earliestCatalogVersion();
+
+ int tableId = tableId(catalogManager, latestCatalogVersion, TABLE_NAME);
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, latestCatalogVersion, latestCatalogVersion),
+ contains(index(catalogManager, latestCatalogVersion, PK_INDEX_NAME))
+ );
+
+ assertThat(
+ collectIndexes(catalogManager, tableId, earliestCatalogVersion, latestCatalogVersion),
+ contains(
+ index(catalogManager, latestCatalogVersion, PK_INDEX_NAME),
+ index(catalogManager, catalogVersionBeforeDropIndex0, indexName0),
+ index(catalogManager, catalogVersionBeforeDropIndex1, indexName1)
+ )
+ );
+ });
+ }
+
+ @Test
+ void testCollectIndexesComplexCase() throws Exception {
Review Comment:
I tried to leave a comment.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405962984
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
Review Comment:
Yes, that for a table there must be at least one index between the requested versions of the catalog.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405960892
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405986795
##########
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/CatalogTestUtils.java:
##########
@@ -298,4 +301,40 @@ public void stop() throws Exception {
}
}
+
+ /**
+ * Searches for a table by name in the requested version of the catalog.
+ *
+ * @param catalogService Catalog service.
+ * @param catalogVersion Catalog version in which to find the table.
+ * @param tableName Table name.
+ */
+ public static CatalogTableDescriptor table(CatalogService catalogService, int catalogVersion, String tableName) {
+ CatalogTableDescriptor tableDescriptor = catalogService.tables(catalogVersion).stream()
+ .filter(table -> tableName.equals(table.name()))
+ .findFirst()
+ .orElse(null);
+
+ assertNotNull(tableDescriptor, "catalogVersion=" + catalogVersion + ", tableName=" + tableName);
+
+ return tableDescriptor;
+ }
+
+ /**
+ * Searches for a index by name in the requested version of the catalog.
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406047101
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
+
+ for (int catalogVersion = catalogVersionFrom; catalogVersion <= catalogVersionTo; catalogVersion++) {
+ for (CatalogIndexDescriptor index : catalogService.indexes(catalogVersion, tableId)) {
+ indexByIdMap.put(index.id(), index);
+ }
+ }
+
+ assert !indexByIdMap.isEmpty()
+ : String.format("catalogVersionFrom=%s, catalogVersionTo=%s, tableId=%s", catalogVersionFrom, catalogVersionTo, tableId);
+
+ var res = new ArrayList<>(indexByIdMap.values());
+
+ res.sort(comparingInt(CatalogObjectDescriptor::id));
+
+ return unmodifiableList(res);
Review Comment:
yep
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406002341
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -444,4 +445,38 @@ private static <T> BiFunction<T, Throwable, CompletableFuture<T>> updater(Functi
return updateFunction.apply(t);
};
}
+
+ /**
+ * Collects indexes (including deleted ones) for tables (tables from the latest version of the catalog) from the earliest to the latest
+ * version of the catalog that need to be started on node recovery.
+ *
+ * @param catalogService Catalog service.
+ */
+ static Map<CatalogTableDescriptor, Collection<CatalogIndexDescriptor>> collectIndexesForRecovery(CatalogService catalogService) {
+ int earliestCatalogVersion = catalogService.earliestCatalogVersion();
+ int latestCatalogVersion = catalogService.latestCatalogVersion();
+
+ var indexesByTableId = new Int2ObjectOpenHashMap<Int2ObjectMap<CatalogIndexDescriptor>>();
+
+ for (CatalogTableDescriptor table : catalogService.tables(latestCatalogVersion)) {
+ indexesByTableId.computeIfAbsent(table.id(), indexById -> new Int2ObjectOpenHashMap<>());
Review Comment:
You're right, I corrected 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406157403
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1405995619
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItIndexManagerTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.index;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.index.IndexManager.collectIndexesForRecovery;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.runAsync;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor;
+import org.apache.ignite.internal.sql.BaseSqlIntegrationTest;
+import org.apache.ignite.internal.table.TableImpl;
+import org.apache.ignite.table.Table;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/** For {@link IndexManager} testing. */
+public class ItIndexManagerTest extends BaseSqlIntegrationTest {
+ private static final String ZONE_NAME = "ZONE_TABLE";
+
+ private static final String TABLE_NAME = "TEST_TABLE";
+
+ private static final String INDEX_NAME = "TEST_INDEX";
+
+ @AfterEach
+ void tearDown() {
+ if (node() != null) {
+ sql("DROP TABLE IF EXISTS " + TABLE_NAME);
+ sql("DROP ZONE IF EXISTS " + ZONE_NAME);
+ }
+ }
+
+ @Override
+ protected int initialNodes() {
+ return 1;
+ }
+
+ @Test
+ void testStartActiveAndDroppedIndexOnNodeRecovery() throws Exception {
+ createZoneAndTable(ZONE_NAME, TABLE_NAME, 1, 1);
+
+ insertPeople(TABLE_NAME, createPeopleBatch(100));
+
+ String indexName0 = INDEX_NAME + 0;
+ String indexName1 = INDEX_NAME + 1;
+
+ createIndex(TABLE_NAME, indexName0, "NAME");
+ createIndex(TABLE_NAME, indexName1, "SALARY");
+
+ awaitIndexesBecomeAvailable(node(), indexName0, indexName1);
+
+ dropIndex(indexName1);
+
+ // Let's restart the node.
+ CLUSTER.stopNode(0);
+ CLUSTER.startNode(0);
+
+ TableImpl tableImpl = getTableImpl(node(), TABLE_NAME);
+
+ assertThat(
+ collectIndexIdsFromTable(tableImpl, 0),
+ equalTo(collectIndexIdsFromCatalogForRecovery(node(), tableImpl))
+ );
+ }
+
+ private static IgniteImpl node() {
+ return CLUSTER.node(0);
+ }
+
+ private static Person[] createPeopleBatch(int batchSize) {
+ return IntStream.range(0, batchSize)
+ .mapToObj(personId -> new Person(personId, "person" + personId, 10.0 + personId))
+ .toArray(Person[]::new);
+ }
+
+ private static TableImpl getTableImpl(IgniteImpl ignite, String tableName) {
+ CompletableFuture<Table> tableFuture = ignite.tables().tableAsync(tableName);
+
+ assertThat(tableFuture, willCompleteSuccessfully());
+
+ return (TableImpl) tableFuture.join();
+ }
+
+ private static List<Integer> collectIndexIdsFromTable(TableImpl table, int partitionId) {
+ CompletableFuture<List<Integer>> future = runAsync(() -> table.indexStorageAdapters(partitionId).get())
Review Comment:
`table.indexStorageAdapters(partitionId).get()`
Because under the hood it uses a join on the same future and can freeze the test.
--
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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406299476
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
+
+ for (int catalogVersion = catalogVersionFrom; catalogVersion <= catalogVersionTo; catalogVersion++) {
+ for (CatalogIndexDescriptor index : catalogService.indexes(catalogVersion, tableId)) {
+ indexByIdMap.put(index.id(), index);
+ }
+ }
+
+ assert !indexByIdMap.isEmpty()
+ : String.format("catalogVersionFrom=%s, catalogVersionTo=%s, tableId=%s", catalogVersionFrom, catalogVersionTo, tableId);
+
+ var res = new ArrayList<>(indexByIdMap.values());
+
+ res.sort(comparingInt(CatalogObjectDescriptor::id));
+
+ return unmodifiableList(res);
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406299107
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param tableId Table ID for which indexes will be collected.
+ * @param catalogVersionFrom Catalog version from which indexes will be collected (including).
+ * @param catalogVersionTo Catalog version up to which indexes will be collected (including).
+ * @return Table indexes sorted by {@link CatalogIndexDescriptor#id()}.
+ */
+ public static List<CatalogIndexDescriptor> collectIndexes(
+ CatalogService catalogService,
+ int tableId,
+ int catalogVersionFrom,
+ int catalogVersionTo
+ ) {
+ assert catalogVersionFrom <= catalogVersionTo : "from=" + catalogVersionFrom + ", to=" + catalogVersionTo;
+
+ if (catalogVersionFrom == catalogVersionTo) {
+ List<CatalogIndexDescriptor> indexes = catalogService.indexes(catalogVersionFrom, tableId);
+
+ assert !indexes.isEmpty() : "catalogVersion=" + catalogVersionFrom + ", tableId=" + tableId;
+
+ return indexes;
+ }
+
+ var indexByIdMap = new Int2ObjectOpenHashMap<CatalogIndexDescriptor>();
Review Comment:
fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406301974
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -399,4 +405,49 @@ static CatalogIndexDescriptor indexOrThrow(Catalog catalog, int indexId) throws
return index;
}
+
+ /**
+ * Collects all table indexes (including dropped) that the table had in the requested catalog version range.
+ *
+ * <p>It is expected that at least one version of the catalog contains table indexes.</p>
Review Comment:
try fix 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-20884 Registering all indexes for active tables on node recovery [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2871:
URL: https://github.com/apache/ignite-3/pull/2871#discussion_r1406049983
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1087,14 +1096,17 @@ private CompletableFuture<?> createTableLocally(long causalityToken, int catalog
* @param zoneDescriptor Catalog distributed zone descriptor.
* @param assignmentsFuture Future with assignments.
* @param catalogVersion Catalog version on which the table was created.
+ * @param onNodeRecovery On node recovery.
Review Comment:
I think you can use a more simple description: `{@code true} when called during node recovery, {@code false} otherwise`
--
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