You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/02/27 21:11:27 UTC

[GitHub] [pulsar] eolivelli commented on a change in pull request #14367: [Broker] Fix Broker HealthCheck Endpoint Exposes Race Conditions

eolivelli commented on a change in pull request #14367:
URL: https://github.com/apache/pulsar/pull/14367#discussion_r815492492



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -55,16 +61,91 @@ public void cleanup() throws Exception {
 
     @Test
     public void testHealthCheckup() throws Exception {
-        admin.brokers().healthcheck();
+        final int times = 30;
+        new Thread(() -> {

Review comment:
       Can we ensure that this Thread is done in the end of the trst? Otherwise we are leaking resources. 
   I would use the Executor provided by the Pulsar service  to run this activity

##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -55,16 +61,91 @@ public void cleanup() throws Exception {
 
     @Test
     public void testHealthCheckup() throws Exception {
-        admin.brokers().healthcheck();
+        final int times = 30;
+        new Thread(() -> {
+            for (int i = 0; i < times; i++) {
+                try {
+                    admin.brokers().healthcheck();
+                    Thread.sleep(20);
+                } catch (PulsarAdminException | InterruptedException e) {
+                    Assert.fail();

Review comment:
       This won't usually work well as assertion errors should escape from the test method.
   You can use a CompletareFuture and complete it exceptionally

##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiHealthCheckTest.java
##########
@@ -55,16 +61,91 @@ public void cleanup() throws Exception {
 
     @Test
     public void testHealthCheckup() throws Exception {
-        admin.brokers().healthcheck();
+        final int times = 30;
+        new Thread(() -> {
+            for (int i = 0; i < times; i++) {
+                try {
+                    admin.brokers().healthcheck();
+                    Thread.sleep(20);

Review comment:
       This test will take many minutes to complete, can we make it shorter?




-- 
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: commits-unsubscribe@pulsar.apache.org

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