[GitHub] [ignite-3] rpuch commented on a diff in pull request #2316: IGNITE-19979 Table start might fail due to an outdated causality token

["rpuch (via GitHub)" <gi...@apache.org>]

rpuch commented on code in PR #2316:
URL: https://github.com/apache/ignite-3/pull/2316#discussion_r1263400164


##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -909,27 +909,27 @@ private CompletableFuture<?> createTablePartitionsLocally(
             return allOf(futures);
         };
 
-        return localPartsByTableIdVv.update(causalityToken, (previous, throwable) -> inBusyLock(busyLock, () -> {
-            return getOrCreatePartitionStorages(table, parts).thenApply(u -> {
-                var newValue = new HashMap<>(previous);
+        // NB: all vv.update() calls must be made from the synchronous part of the method (not in thenCompose()/etc!).
+        CompletableFuture<?> localPartsUpdateFuture = localPartsByTableIdVv.update(causalityToken,
+                (previous, throwable) -> inBusyLock(busyLock, () -> {
+                    return getOrCreatePartitionStorages(table, parts).thenApply(u -> {
+                        var newValue = new HashMap<>(previous);
 
-                newValue.put(tableId, parts);
+                        newValue.put(tableId, parts);
 
-                return newValue;
-            });
-        })).thenCompose(unused -> {
-            CompletableFuture<Void> updateAssignmentsFuture = tablesByIdVv.get(causalityToken).thenComposeAsync(
-                    tablesById -> inBusyLock(busyLock, updateAssignmentsClosure),
-                    ioExecutor
-            );
+                        return newValue;
+                    });
+                }));
 
-            return assignmentsUpdatedVv.update(causalityToken, (token, e) -> {
-                if (e != null) {
-                    return failedFuture(e);
-                }
+        return assignmentsUpdatedVv.update(causalityToken, (token, e) -> {
+            if (e != null) {
+                return failedFuture(e);
+            }
 
-                return updateAssignmentsFuture;
-            });
+            return localPartsUpdateFuture.thenCompose(unused ->

Review Comment:
   If I change the code to the following
   
   ```
           return assignmentsUpdatedVv.update(causalityToken, (token, e) -> {
               if (e != null) {
                   return failedFuture(e);
               }
   
               return tablesByIdVv.get(causalityToken)
                       .thenComposeAsync(tablesById -> inBusyLock(busyLock, updateAssignmentsClosure), ioExecutor);
           });
   ```
   , I get an assertion:
   
   ```
   Caused by: java.lang.AssertionError
   	at org.apache.ignite.internal.table.distributed.TableManager.getPartitionStorages(TableManager.java:2502)
   	at org.apache.ignite.internal.table.distributed.TableManager.lambda$createTablePartitionsLocally$13(TableManager.java:763)
   	at org.apache.ignite.internal.util.IgniteUtils.inBusyLock(IgniteUtils.java:893)
   	at org.apache.ignite.internal.table.distributed.TableManager.lambda$createTablePartitionsLocally$17(TableManager.java:930)
   	at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1072)
   	... 4 more
   ```
   
   Also, `tablesByIdVv.get(causalityToken)` is not done when `assignmentsUpdatedVv`-related callback is called, so the assertion cannot be added.



-- 
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