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/03/28 14:56:02 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
tkalkirill opened a new pull request, #1855:
URL: https://github.com/apache/ignite-3/pull/1855
https://issues.apache.org/jira/browse/IGNITE-19085
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151469795
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
Before recovery raft groups, it is important for us to know the list of indices that will be updated on recovery (in `PartitionListener`), now we are waiting only for the PK index, and we forget about the rest, which is not correct.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151474206
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
can you please describe step-by-step what happens during recovery? I still don't understand why is this code located in the assignments change handler and how it is related to the "recovery raft groups" you are talking about
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151466846
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
Review Comment:
No, right now the deletion of the index is not implemented correctly in general and not tested as it should, I will create a ticket for this.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151486567
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
Review Comment:
Add 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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151465857
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -413,4 +392,24 @@ private interface IndexStorageAdapterFactory {
/** Creates the index decorator for given partition. */
TableSchemaAwareIndexStorage create(int partitionId);
}
+
+ /**
+ * Adds indexes to wait before inserting data into the table.
+ *
+ * @param indexIds Indexes Index IDs.
+ */
+ // TODO: IGNITE-19082 Needs to be redone/improved
Review Comment:
Right now I'm close with the `IndexManager`, it should finish waiting for the indexes, since it will be removed in this ticket, then its author will need to take care of this problem.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151511178
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
Discussed personally, I think that the problem has become clearer.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151470888
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
- for (UUID indexId : indexIds) {
- if (indexLockerFactories.containsKey(indexId) && indexStorageAdapterFactories.containsKey(indexId)) {
Review Comment:
Can you clarify please?
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151791556
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1106,7 +1107,10 @@ private CompletableFuture<?> createTableLocally(long causalityToken, String name
partitions, clusterNodeResolver, txManager, tableStorage, txStateStorage, replicaSvc, clock);
// TODO: IGNITE-16288 directIndexIds should use async configuration API
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151505163
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
After an in-person discussion we decided that this code should actually be moved to the table creation listeners
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151469795
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
In `onUpdateAssignments`, we start creating a table, this is a convenient entry point for adding indexes before recover raft groups.
Why not do it on table creation listeners?
**I'll try to answer:** Creating (each) indexes is an asynchronous operation that can be performed later than we prepared to recover the raft groups and we can skip the indexes at the beginning of the recovery of the raft group.
Before recovery raft groups, it is important for us to know the list of indices that will be updated on recovery (in `PartitionListener`), now we are waiting only for the PK index, and we forget about the rest, which is not correct.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151496654
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
- for (UUID indexId : indexIds) {
- if (indexLockerFactories.containsKey(indexId) && indexStorageAdapterFactories.containsKey(indexId)) {
Review Comment:
ok, I've read the code more carefully and looks like this check doesn't make much sense indeed
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151467452
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
- for (UUID indexId : indexIds) {
- if (indexLockerFactories.containsKey(indexId) && indexStorageAdapterFactories.containsKey(indexId)) {
Review Comment:
For me, this check does not make sense at the moment.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151470294
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
Review Comment:
Please add a TODO 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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151533877
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1106,7 +1107,10 @@ private CompletableFuture<?> createTableLocally(long causalityToken, String name
partitions, clusterNodeResolver, txManager, tableStorage, txStateStorage, replicaSvc, clock);
// TODO: IGNITE-16288 directIndexIds should use async configuration API
Review Comment:
Looks like this TODO is not relevant anymore
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151455461
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
Review Comment:
Just an array will not work for me, since the size of a `ConcurrentHashMap` can change at any time.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151471870
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -413,4 +392,24 @@ private interface IndexStorageAdapterFactory {
/** Creates the index decorator for given partition. */
TableSchemaAwareIndexStorage create(int partitionId);
}
+
+ /**
+ * Adds indexes to wait before inserting data into the table.
+ *
+ * @param indexIds Indexes Index IDs.
+ */
+ // TODO: IGNITE-19082 Needs to be redone/improved
Review Comment:
I'm just checking that the author is aware of the problem you are trying to fix and knows about this TODO
--
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
[GitHub] [ignite-3] tkalkirill merged pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151433972
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -413,4 +392,24 @@ private interface IndexStorageAdapterFactory {
/** Creates the index decorator for given partition. */
TableSchemaAwareIndexStorage create(int partitionId);
}
+
+ /**
+ * Adds indexes to wait before inserting data into the table.
+ *
+ * @param indexIds Indexes Index IDs.
+ */
+ // TODO: IGNITE-19082 Needs to be redone/improved
+ public void addIndexesToWait(Collection<UUID> indexIds) {
+ for (UUID indexId : indexIds) {
+ indexesToWait.computeIfAbsent(indexId, uuid -> new CompletableFuture<>());
+ }
+ }
+
+ private void completeRegisterIndex(UUID indexId) {
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.computeIfAbsent(indexId, uuid -> new CompletableFuture<>());
Review Comment:
It used to be `remove` but now its `computeIfAbsent`, why?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
Review Comment:
Do we test this behavior anywhere? Shouldn't this future be completed with an exception? How does this scenario work?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
Review Comment:
Just a nitpick, but you can use an array directly, without an intermediate `ArrayList`
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
Why is this code located in `onUpdateAssignments` and not in table or index creation listeners?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2345,4 +2350,20 @@ private static void handleExceptionOnCleanUpTablesResources(
nodeStoppingEx.set(true);
}
}
+
+ private static Collection<UUID> collectTableIndexes(TablesView tablesConfig, UUID tableId) {
Review Comment:
Another little nitpick: this method does not have to be `static`, since `tablesConfig` is a field of this class and can be removed from parameters
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -413,4 +392,24 @@ private interface IndexStorageAdapterFactory {
/** Creates the index decorator for given partition. */
TableSchemaAwareIndexStorage create(int partitionId);
}
+
+ /**
+ * Adds indexes to wait before inserting data into the table.
+ *
+ * @param indexIds Indexes Index IDs.
+ */
+ // TODO: IGNITE-19082 Needs to be redone/improved
Review Comment:
Did you discuss this with @AMashenkov (the author of this ticket) and that it will indeed be done under this ticket?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
- for (UUID indexId : indexIds) {
- if (indexLockerFactories.containsKey(indexId) && indexStorageAdapterFactories.containsKey(indexId)) {
Review Comment:
Why did you remove this check?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -673,15 +675,18 @@ private CompletableFuture<?> updateAssignmentInternal(ConfigurationNotificationE
return failedFuture(e);
}
+ TableImpl table = tablesById.get(tblId);
+
+ // TODO: IGNITE-19082 Need another way to wait for indexes
+ table.addIndexesToWait(collectTableIndexes(tablesCfg.value(), tblId));
Review Comment:
Why is this code even needed? I can't understand this from the ticket description and we already populate in index-to-wait map when registering indices
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151457682
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -413,4 +392,24 @@ private interface IndexStorageAdapterFactory {
/** Creates the index decorator for given partition. */
TableSchemaAwareIndexStorage create(int partitionId);
}
+
+ /**
+ * Adds indexes to wait before inserting data into the table.
+ *
+ * @param indexIds Indexes Index IDs.
+ */
+ // TODO: IGNITE-19082 Needs to be redone/improved
+ public void addIndexesToWait(Collection<UUID> indexIds) {
+ for (UUID indexId : indexIds) {
+ indexesToWait.computeIfAbsent(indexId, uuid -> new CompletableFuture<>());
+ }
+ }
+
+ private void completeRegisterIndex(UUID indexId) {
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.computeIfAbsent(indexId, uuid -> new CompletableFuture<>());
Review Comment:
In general, it can be redone for removal.
I will do so.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151453461
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2345,4 +2350,20 @@ private static void handleExceptionOnCleanUpTablesResources(
nodeStoppingEx.set(true);
}
}
+
+ private static Collection<UUID> collectTableIndexes(TablesView tablesConfig, UUID tableId) {
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151469897
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
Review Comment:
ok, makes sense
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1855: IGNITE-19085 Fix waiting for indexes on recovery when building indexes
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1855:
URL: https://github.com/apache/ignite-3/pull/1855#discussion_r1151491985
##########
modules/table/src/main/java/org/apache/ignite/internal/table/TableImpl.java:
##########
@@ -361,33 +348,21 @@ public void registerSortedIndex(
public void unregisterIndex(UUID indexId) {
indexLockerFactories.remove(indexId);
indexStorageAdapterFactories.remove(indexId);
+
+ CompletableFuture<?> indexToWaitFuture = indexesToWait.remove(indexId);
+
+ if (indexToWaitFuture != null) {
+ indexToWaitFuture.complete(null);
+ }
}
private void awaitIndexes() {
// TODO: replace with actual call to ids supplier
- List<UUID> indexIds = List.of(pkId()); // activeIndexIds.get();
-
List<CompletableFuture<?>> toWait = new ArrayList<>();
- for (UUID indexId : indexIds) {
- if (indexLockerFactories.containsKey(indexId) && indexStorageAdapterFactories.containsKey(indexId)) {
Review Comment:
If the index is registered, then the future of waiting for the indexes will not be or completed, so there is no need to check the maps.
--
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