[GitHub] [zookeeper] eolivelli commented on a change in pull request #1821: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest.testLargeRequestThrottling

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #1821:
URL: https://github.com/apache/zookeeper/pull/1821#discussion_r814908906



##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -277,34 +286,32 @@ public void testLargeRequestThrottling() throws Exception {
         resumeProcess = new CountDownLatch(1);
         // the connection will be close when large requests exceed the limit
         // we can't use the submitted latch because requests after close won't be submitted
-        disconnected = new CountDownLatch(TOTAL_REQUESTS);
-
-        // the total length of the request is about 170-180 bytes, so only two requests are allowed
-        byte[] data = new byte[100];
+        disconnected = new CountDownLatch(number_requests);
 
-        // send 5 requests asynchronously
-        for (int i = 0; i < TOTAL_REQUESTS; i++) {
+        // send requests asynchronously
+        for (int i = 0; i < number_requests; i++) {
             zk.create("/request_throttle_test- " + i , data,
                     ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, createCallback, null);
         }
 
-        // make sure the server received all 5 requests
-        disconnected.await(30, TimeUnit.SECONDS);
+        // make sure the server received all requests
+        assertTrue(disconnected.await(30, TimeUnit.SECONDS));
+
+        finished = new CountDownLatch(2);
+        // let the requests go through the pipeline
+        resumeProcess.countDown();
+        assertTrue(finished.await(5, TimeUnit.SECONDS));
+
+        // assert metrics after finished so metrics in no io threads are set also.
         Map<String, Object> metrics = MetricsUtils.currentServerMetrics();
 
         // but only two requests can get into the pipeline because they are large requests
         // the connection will be closed
         assertEquals(2L, (long) metrics.get("prep_processor_request_queued"));
         assertEquals(1L, (long) metrics.get("large_requests_rejected"));
-        assertEquals(5, connectionLossCount);
-
-        finished = new CountDownLatch(2);
-        // let the requests go through the pipeline
-        resumeProcess.countDown();
-        finished.await(5, TimeUnit.SECONDS);
+        assertEquals(number_requests, connectionLossCount);
 
         // when the two requests finish, they are stale because the connection is closed already
-        metrics = MetricsUtils.currentServerMetrics();

Review comment:
       Why we are not collecting the metrics here?




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

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