rkhachatryan commented on a change in pull request #17462:
URL: https://github.com/apache/flink/pull/17462#discussion_r749136798



##########
File path: 
flink-state-backends/flink-statebackend-changelog/src/test/java/org/apache/flink/state/changelog/ChangelogStateBackendTestUtils.java
##########
@@ -158,16 +155,15 @@ public static void testMaterializedRestore(
             keyedBackend.setCurrentKey(2);
             state.update(new StateBackendTestBase.TestPojo("u2", 2));
 
-            awaitMaterialization(keyedBackend, env.getMainMailboxExecutor());
+            // trigger materialization manually
+            periodicMaterializationManager.triggerMaterialization();
 
             keyedBackend.setCurrentKey(2);
             state.update(new StateBackendTestBase.TestPojo("u2", 22));
 
             keyedBackend.setCurrentKey(3);
             state.update(new StateBackendTestBase.TestPojo("u3", 3));
 
-            awaitMaterialization(keyedBackend, env.getMainMailboxExecutor());
-

Review comment:
       I think it's very fragile to rely on 
   ```
   this.asyncOperationsThreadPool = Executors.newDirectExecutorService();
   ```
   in constructor of a different class (`MockEnvironment`) in a different 
module.
   That class is also used in many other tests. This change adds an 
**implicit** contract that its default `asyncOperationsThreadPool` **must** be 
direct.
   
   Could you explain what was the motivation for the change in the first place?




-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to