kirklund commented on a change in pull request #5014: URL: https://github.com/apache/geode/pull/5014#discussion_r420945761
########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/GemFireCacheImplTest.java ########## @@ -620,6 +625,40 @@ public void getCacheServers_isCanonical() { .isSameAs(gemFireCacheImpl.getCacheServers()); } + @Test + public void testLockDiskStore() throws InterruptedException { + int nThread = 10; + String diskStoreName = "MyDiskStore"; + AtomicInteger nTrue = new AtomicInteger(); + AtomicInteger nFalse = new AtomicInteger(); + ExecutorService executorService = Executors.newFixedThreadPool(nThread); Review comment: I recommend replacing ExecutorService with ExecutorServiceRule. You can limit the rule to a specific number of threads if you need to otherwise it defaults to as many threads as tasks that you submit: ``` @Rule public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule(); ``` The Rule will automatically do shutdown etc during tearDown(). Also, you might want to capture the `Future<Void>` return value from `executorService/executorServiceRule.submit` to await on. You could even have a `Collection<Future<Void>>` if you wanted. If you await on the Futures, then any assertion failures will be thrown causing the test to fail: ``` Future<Void> doLockUnlock = executorService.submit(() -> { try { assertThat(gemFireCacheImpl.doLockDiskStore(diskStoreName)).isTrue(); } finally { assertThat(gemFireCacheImpl. doUnlockDiskStore(diskStoreName)).isTrue(); } } doLockUnlock.get(GeodeAwaitility.getTimeout().toMillis(), TimeUnit.MILLISECONDS); ``` ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org