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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
