adoroszlai commented on code in PR #10680:
URL: https://github.com/apache/ozone/pull/10680#discussion_r3533905500
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java:
##########
@@ -453,24 +452,34 @@ void testMultiLockResourceParallel() throws Exception {
lock.acquireMultiUserLock("user2", "user1");
AtomicBoolean gotLock = new AtomicBoolean(false);
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
lock.acquireMultiUserLock("user1", "user2");
gotLock.set(true);
lock.releaseMultiUserLock("user1", "user2");
- }).start();
- // Let's give some time for the new thread to run
- Thread.sleep(100);
+ });
+ thread.start();
+ waitForBlockedThread(thread);
// Since the new thread is trying to get lock on same resource, it will
// wait.
assertFalse(gotLock.get());
lock.releaseMultiUserLock("user2", "user1");
// Since we have released the lock, the new thread should have the lock
// now.
- // Let's give some time for the new thread to run
- Thread.sleep(100);
+ waitForThread(thread);
assertTrue(gotLock.get());
}
+ private static void waitForBlockedThread(Thread thread) throws Exception {
+ GenericTestUtils.waitFor(() -> thread.getState() == Thread.State.BLOCKED ||
+ thread.getState() == Thread.State.WAITING ||
+ thread.getState() == Thread.State.TIMED_WAITING, 10, 10000);
Review Comment:
nit: `getState()` once
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java:
##########
@@ -1422,6 +1449,12 @@ private long getRunCount() {
return count;
}
+ private static void waitFor(CheckedSupplier<Boolean, RuntimeException> check,
+ int checkEveryMillis, int waitForMillis)
+ throws TimeoutException, InterruptedException {
+ GenericTestUtils.waitFor(check, checkEveryMillis, waitForMillis);
Review Comment:
Can this be replaced with `import static ...GenericTestUtils.waitFor`?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java:
##########
@@ -453,24 +452,34 @@ void testMultiLockResourceParallel() throws Exception {
lock.acquireMultiUserLock("user2", "user1");
AtomicBoolean gotLock = new AtomicBoolean(false);
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
lock.acquireMultiUserLock("user1", "user2");
gotLock.set(true);
lock.releaseMultiUserLock("user1", "user2");
- }).start();
- // Let's give some time for the new thread to run
- Thread.sleep(100);
+ });
+ thread.start();
+ waitForBlockedThread(thread);
// Since the new thread is trying to get lock on same resource, it will
// wait.
assertFalse(gotLock.get());
lock.releaseMultiUserLock("user2", "user1");
// Since we have released the lock, the new thread should have the lock
// now.
- // Let's give some time for the new thread to run
- Thread.sleep(100);
+ waitForThread(thread);
assertTrue(gotLock.get());
}
+ private static void waitForBlockedThread(Thread thread) throws Exception {
+ GenericTestUtils.waitFor(() -> thread.getState() == Thread.State.BLOCKED ||
+ thread.getState() == Thread.State.WAITING ||
+ thread.getState() == Thread.State.TIMED_WAITING, 10, 10000);
+ }
+
+ private static void waitForThread(Thread thread) throws InterruptedException
{
+ thread.join(TimeUnit.SECONDS.toMillis(10));
+ assertFalse(thread.isAlive(), "Timed out waiting for lock thread to
finish");
Review Comment:
nit: `waitForFinish` would be a better name?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -375,11 +402,17 @@ public void testContentsOfTarballWithSnapshot(boolean
includeSnapshot) throws Ex
assertFalse(inodesFromTarball.isEmpty());
assertTrue(inodesFromTarball.containsAll(inodesFromOmDataDir));
- long actualYamlFiles = Files.list(newDbDir.toPath())
- .filter(f -> f.getFileName().toString()
- .endsWith(".yaml")).count();
- assertEquals(numSnapshots, actualYamlFiles,
- "Number of generated YAML files should match the number of
snapshots.");
+ long actualYamlFiles;
+ try (Stream<Path> files = Files.list(newDbDir.toPath())) {
+ actualYamlFiles = files.filter(f ->
f.getFileName().toString().endsWith(".yaml")).count();
+ }
+ if (includeSnapshot) {
+ assertTrue(actualYamlFiles >= numSnapshots,
+ "Generated YAML files should include this test's snapshots.");
Review Comment:
nit: please
`assertThat(actualYamlFiles).isGreaterThanOrEqualTo(numSnapshots)`, see
HDDS-9951
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
