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/05/05 23:59:06 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3384: fix 3372 - use waitFor to handle async conditions
EdColeman opened a new pull request, #3384:
URL: https://github.com/apache/accumulo/pull/3384
Update test to use Wait.waitFor that should be more resilient for testing async metrics.
--
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 pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#issuecomment-1542147037
The memory starved tests were added in 3.0 with https://github.com/apache/accumulo/pull/3161
--
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 closed pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman closed pull request #3384: fix 3372 - use waitFor to handle async conditions
URL: https://github.com/apache/accumulo/pull/3384
--
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 #3384: fix 3372 - use waitFor to handle async conditions
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1189776103
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -272,7 +273,7 @@ public void testScanPauses() throws Exception {
Thread.sleep(1500);
assertEquals(currentCount, fetched.get());
- assertEquals(1, LOW_MEM_DETECTED.get());
+ assertTrue(Wait.waitFor(() -> (LOW_MEM_DETECTED.get() == 1), 20_000L, 1000L));
Review Comment:
All of these lines use the same max wait time and delay, and all of them assert that the condition is true (that assertion could probably be moved into the Wait class.. but I didn't check to see where all it was used). All of them also seem to have an unnecessary parens around the condition in the lambda. You should remove all this boilerplate by creating a local method:
```suggestion
waitForTrue(() -> LOW_MEM_DETECTED.get() == 1);
```
The method would look like:
```java
private static void waitForTrue(Wait.Condition condition) throws Exception {
// could specify 20 seconds, but the default is 30, and that's fine. The default delay is 1.
assertTrue(Wait.waitFor(condition));
}
```
--
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 pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#issuecomment-1542050282
Is this applicable to 2.1, or only 3.0?
--
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 pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#issuecomment-1598880557
Superseded with #3508. The test reveled an issue with the low memory detector not necessarily the test code.
--
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 #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1187512903
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -307,13 +308,16 @@ public void testBatchScanReturnsEarlyDueToLowMemory() throws Exception {
consumeServerMemory(scanner);
// Wait for longer than the memory check interval
- Thread.sleep(6000);
+ Thread.sleep(3000);
Review Comment:
The metrics collection is inherently asynchronous - this was to insert some delay and then use the polling in Wait so that the back off could start small and then start backing off, hopefully reducing the over-all time required for the test by minimizing the total time spent waiting for the metrics to update.
This test in particular seems to have a race condition with the low memory detector that was recently added - so it was the only one addressed here. The others were left alone for now
--
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] cshannon commented on a diff in pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1186710068
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -307,13 +308,16 @@ public void testBatchScanReturnsEarlyDueToLowMemory() throws Exception {
consumeServerMemory(scanner);
// Wait for longer than the memory check interval
- Thread.sleep(6000);
+ Thread.sleep(3000);
Review Comment:
Why do you still have a sleep here? The main point of using the Wait.waitFor() api is you don't need to include anymore artificial sleeps as it will progress when ready.
--
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] cshannon commented on a diff in pull request #3384: fix 3372 - use waitFor to handle async conditions
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1186710772
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -377,31 +381,37 @@ public void testBatchScanPauses() throws Exception {
// Grab the current paused count, wait two seconds and then confirm that
// the number of rows fetched by the memoryConsumingScanner has not increased
// and that the scan delay counter has increased.
- double returned = SCAN_RETURNED_EARLY.doubleValue();
- double paused = SCAN_START_DELAYED.doubleValue();
+ final double returned = SCAN_RETURNED_EARLY.doubleValue();
+ final double paused = SCAN_START_DELAYED.doubleValue();
Thread.sleep(1500);
assertEquals(currentCount, fetched.get());
- assertTrue(SCAN_START_DELAYED.doubleValue() >= paused);
- assertTrue(SCAN_RETURNED_EARLY.doubleValue() >= returned);
- assertEquals(1, LOW_MEM_DETECTED.get());
+
+ assertTrue(
+ Wait.waitFor(() -> (SCAN_START_DELAYED.doubleValue() >= paused), 20_000L, 1000L));
+ assertTrue(
+ Wait.waitFor(() -> (SCAN_RETURNED_EARLY.doubleValue() >= returned), 20_000L, 1000L));
+ assertTrue(Wait.waitFor(() -> (LOW_MEM_DETECTED.get() == 1), 20_000L, 1000L));
// Perform the check again
- paused = SCAN_START_DELAYED.doubleValue();
- returned = SCAN_RETURNED_EARLY.doubleValue();
+ final double paused2 = SCAN_START_DELAYED.doubleValue();
+ final double returned2 = SCAN_RETURNED_EARLY.doubleValue();
Thread.sleep(1500);
assertEquals(currentCount, fetched.get());
- assertTrue(SCAN_START_DELAYED.doubleValue() >= paused);
- assertEquals(returned, SCAN_RETURNED_EARLY.doubleValue());
- assertEquals(1, LOW_MEM_DETECTED.get());
+
+ assertTrue(
+ Wait.waitFor(() -> (SCAN_START_DELAYED.doubleValue() >= paused2), 20_000L, 1000L));
+ assertTrue(
+ Wait.waitFor(() -> (SCAN_RETURNED_EARLY.doubleValue() == returned2), 20_000L, 1000L));
+ assertTrue(Wait.waitFor(() -> (LOW_MEM_DETECTED.get() == 1), 20_000L, 1000L));
// Free the memory which will allow the pausing scanner to continue
freeServerMemory(client, table);
t.join();
assertEquals(30, fetched.get());
// allow metic collection to cycle.
- Thread.sleep(6_000);
- assertEquals(0, LOW_MEM_DETECTED.get());
+ Thread.sleep(3_000);
Review Comment:
Another case where the sleep can be removed
--
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 #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1187513344
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -307,13 +308,16 @@ public void testBatchScanReturnsEarlyDueToLowMemory() throws Exception {
consumeServerMemory(scanner);
// Wait for longer than the memory check interval
- Thread.sleep(6000);
+ Thread.sleep(3000);
Review Comment:
The metrics collection is inherently asynchronous - this was to insert some delay and then use the polling in Wait so that the back off could start small and then start backing off, hopefully reducing the over-all time required for the test by minimizing the total time spent waiting for the metrics to update.
This test in particular seems to have a race condition with the low memory detector that was recently added - so it was the only one addressed here. The others were left alone for now
--
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 #3384: fix 3372 - use waitFor to handle async conditions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3384:
URL: https://github.com/apache/accumulo/pull/3384#discussion_r1187513344
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -307,13 +308,16 @@ public void testBatchScanReturnsEarlyDueToLowMemory() throws Exception {
consumeServerMemory(scanner);
// Wait for longer than the memory check interval
- Thread.sleep(6000);
+ Thread.sleep(3000);
Review Comment:
The metrics collection is inherently asynchronous - this was to insert some delay and then use the polling in Wait so that the back off could start small and then start backing off, hopefully reducing the over-all time required for the test by minimizing the total time spent waiting for the metrics to update.
This test in particular seems to have a race condition with the low memory detector that was recently added - so it was the only one addressed here. The others were left alone for now
--
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