[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests

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

ctubbsii commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1124945656


##########
test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java:
##########
@@ -296,7 +295,12 @@ public void testCompactEmptyTablesWithBadIterator_FailsAndCancel() throws TableE
     list.add(new IteratorSetting(15, BadIterator.class));
     accumuloClient.tableOperations().compact(tableName, null, null, list, true, false); // don't
                                                                                         // block
-    sleepUninterruptibly(2, TimeUnit.SECONDS); // start compaction
+    try {
+      Thread.sleep(SECONDS.toMillis(2)); // start compaction
+    } catch (InterruptedException ex) {
+      Thread.currentThread().interrupt();
+      throw new AccumuloException("interrupted during sleep", ex);
+    }

Review Comment:
   You can just make the `@Test` method throw the `InterruptedException`. We don't need to catch it and re-throw as a different type.



##########
core/src/test/java/org/apache/accumulo/core/clientImpl/bulk/ConcurrentKeyExtentCacheTest.java:
##########
@@ -83,8 +80,12 @@ protected Stream<KeyExtent> lookupExtents(Text row) {
         }
       }
 
-      Uninterruptibles.sleepUninterruptibly(3, MILLISECONDS);
-
+      try {
+        Thread.sleep(3);
+      } catch (InterruptedException ex) {
+        Thread.currentThread().interrupt();
+        throw new IllegalStateException("interrupted during sleep", ex);
+      }

Review Comment:
   Even if the test does get interrupted, it's because we're killing the whole JVM. Even if the JVM is delayed by 3 milliseconds, I don't think this is going to be a problem, and the previous code is much more succinct.



##########
test/src/main/java/org/apache/accumulo/test/functional/ScanIdIT.java:
##########
@@ -316,7 +315,12 @@ private void addSplits(final AccumuloClient client, final String tableName) {
 
       client.tableOperations().offline(tableName, true);
 
-      sleepUninterruptibly(2, TimeUnit.SECONDS);
+      try {
+        Thread.sleep(SECONDS.toMillis(2));
+      } catch (InterruptedException ex) {
+        Thread.currentThread().interrupt();
+        throw new IllegalStateException("interrupted during sleep", ex);
+      }

Review Comment:
   Could just let the exception fall through to the test method instead of trying to catch it and rethrow as another type. It makes it a bit more readable for the test method.



##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -1584,19 +1583,33 @@ public void verifyPerTableClasspath(final String table, final File fooConstraint
         "config -t " + table + " -s " + Property.TABLE_CLASSLOADER_CONTEXT.getKey() + "=" + context,
         true);
 
-    sleepUninterruptibly(250, TimeUnit.MILLISECONDS);
-
+    try {
+      Thread.sleep(250);
+    } catch (InterruptedException ex) {

Review Comment:
   This could probably just be allowed to fall outside the `@Test` method that called this helper method. Same with the other new catch blocks in this file.



##########
test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompactionProgressIT.java:
##########
@@ -113,7 +111,12 @@ public Thread startChecker() {
       try {
         while (!compactionFinished.get()) {
           checkRunning();
-          sleepUninterruptibly(1000, TimeUnit.MILLISECONDS);
+          try {
+            Thread.sleep(1000);
+          } catch (InterruptedException ex) {
+            Thread.currentThread().interrupt();
+            throw new IllegalStateException("interrupted during sleep", ex);
+          }

Review Comment:
   It might be better to set `compactionFinished.set(true)` if this thread is interrupted and allow it to finish gracefully, rather than throw the IllegalStateException.



-- 
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@accumulo.apache.org

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