You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/03/03 17:18:22 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
EdColeman opened a new pull request, #3225:
URL: https://github.com/apache/accumulo/pull/3225
This is a subset (and supersedes) https://github.com/apache/accumulo/pull/3202 and focuses on replacements only in test code.
- replace occurrences of sleepUninterruptibly with Thread.sleep()
- prefer static import of TimeUnits instead of using TimeUnit.[UNIT] in files that had changes.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1125059907
##########
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:
Modified in 4ae251cf0a
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1126497130
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -39,7 +38,12 @@ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> op
MemoryConsumingIterator.freeBuffers();
while (this.isRunningLowOnMemory()) {
// wait for LowMemoryDetector to recognize the memory is free.
- Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+ try {
+ Thread.sleep(SECONDS.toMillis(1));
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
Review Comment:
The fact that it is never checked is likely true. In that case, resenting it will have no effect up the stack. Not resetting it prevents the possibility that it *will never be* handled better. Resetting it is a better practice, so why not?
--
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
[GitHub] [accumulo] EdColeman merged pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3225:
URL: https://github.com/apache/accumulo/pull/3225
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1125059683
##########
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:
Modified in 4ae251cf0a
##########
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:
Modified in 4ae251cf0a
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1126497130
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -39,7 +38,12 @@ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> op
MemoryConsumingIterator.freeBuffers();
while (this.isRunningLowOnMemory()) {
// wait for LowMemoryDetector to recognize the memory is free.
- Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+ try {
+ Thread.sleep(SECONDS.toMillis(1));
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
Review Comment:
The fact that it is never checked is likely true. In that case, resetting it will have no effect up the stack. Not resetting it prevents the possibility that it *will never be* handled better. Resetting it is a better practice, so why not?
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1126394679
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -39,7 +38,12 @@ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> op
MemoryConsumingIterator.freeBuffers();
while (this.isRunningLowOnMemory()) {
// wait for LowMemoryDetector to recognize the memory is free.
- Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
+ try {
+ Thread.sleep(SECONDS.toMillis(1));
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
Review Comment:
I think these additions are going unchecked in the tests. `MemoryFreeingIterator` and `MemoryConsumingIterator` are used in the MemoryStarved*IT's. I don't think you can check them there as these are scan iterators. I don't think there is anything in the server-side scan code that checks this condition and relays it back to the client.
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3225: replace sleepUninterruptibly() with Thread.sleep() in tests
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3225:
URL: https://github.com/apache/accumulo/pull/3225#discussion_r1125059495
##########
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:
Modified in 4ae251cf0a
##########
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:
Modified in 4ae251cf0a
--
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