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 2021/06/29 14:03:11 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Technoboy- opened a new pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150
1. Fix #11149 .
2. Fix LOAD_REPORT_UPDATE_MIMIMUM_INTERVAL -> LOAD_REPORT_UPDATE_MINIMUM_INTERVAL.
3. Fix getDynamicConfigurationDouble and getDynamicConfigurationBoolean with no defaultValue.
--
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
[GitHub] [pulsar] Technoboy- commented on pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#issuecomment-871158877
ping @codelipenghui
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661280132
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -962,10 +966,15 @@ public void accept(Notification n) {
log.debug("Received updated load report from broker node - [{}], scheduling re-ranking of brokers.",
n.getPath());
}
- scheduler.submit(this::updateRanking);
+ updateRanking = scheduler.submit(this::updateRanking);
}
}
+ @VisibleForTesting
+ Future<?> getUpdateRanking(){
+ return updateRanking;
Review comment:
what about `updateRankingHandle` ?
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,11 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRanking");
+ Object invoke = method.invoke(pulsarServices[i].getLoadManager().get());
Review comment:
this line should be moved inside the `while` loop
other wise if `invoke` is null it will be an endless loop
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -962,10 +966,15 @@ public void accept(Notification n) {
log.debug("Received updated load report from broker node - [{}], scheduling re-ranking of brokers.",
n.getPath());
}
- scheduler.submit(this::updateRanking);
+ updateRanking = scheduler.submit(this::updateRanking);
}
}
+ @VisibleForTesting
+ Future<?> getUpdateRanking(){
Review comment:
you make make this "public" , it is an "Impl" class, it is not public API
--
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
[GitHub] [pulsar] Technoboy- commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661400384
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,11 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRanking");
+ Object invoke = method.invoke(pulsarServices[i].getLoadManager().get());
Review comment:
yes, thanks, right.
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661405614
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,12 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRankingHandle");
+ Object invoke = method.invoke(pulsarServices[i].getLoadManager().get());
Review comment:
Sorry, I could have suggested it before...
We recently started to use Awaiatility for this kind of waits
What about rewriting the loop this way?
```
Awaiatility.await().until( () -> {
Object invoke = method.invoke(pulsarServices[i].getLoadManager().get());
return invoke != null && invoke.isDone();
});
```
--
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
[GitHub] [pulsar] Technoboy- commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661400089
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -962,10 +966,15 @@ public void accept(Notification n) {
log.debug("Received updated load report from broker node - [{}], scheduling re-ranking of brokers.",
n.getPath());
}
- scheduler.submit(this::updateRanking);
+ updateRanking = scheduler.submit(this::updateRanking);
}
}
+ @VisibleForTesting
+ Future<?> getUpdateRanking(){
Review comment:
Ok
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -962,10 +966,15 @@ public void accept(Notification n) {
log.debug("Received updated load report from broker node - [{}], scheduling re-ranking of brokers.",
n.getPath());
}
- scheduler.submit(this::updateRanking);
+ updateRanking = scheduler.submit(this::updateRanking);
}
}
+ @VisibleForTesting
+ Future<?> getUpdateRanking(){
+ return updateRanking;
Review comment:
Ok
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661216170
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -319,7 +323,7 @@ private String getDynamicConfigurationFromStore(String path, String settingName,
private double getDynamicConfigurationDouble(String path, String settingName, double defaultValue) {
double result = defaultValue;
try {
- String setting = this.getDynamicConfigurationFromStore(path, settingName, null);
+ String setting = this.getDynamicConfigurationFromStore(path, settingName, String.valueOf(defaultValue));
Review comment:
what is the impact of this change ?
we should add unit tests around this method and `getDynamicConfigurationBoolean`
--
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
[GitHub] [pulsar] Technoboy- commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661456584
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,12 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRankingHandle");
+ final int index = i;
+ Awaitility.await().until(() -> {
+ Object invoke = method.invoke(pulsarServices[index].getLoadManager().get());
Review comment:
yes, right.
--
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
[GitHub] [pulsar] Technoboy- commented on pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#issuecomment-871192988
> great to see the test fixed.
> thank you !
>
> I am concerned with the change about dynamic configuration.
> do we need that change in order to make the test less flaky ?
>
> otherwise I believe it is better to split the patch into two parts
Thanks, I will split the patch, update later.
--
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
[GitHub] [pulsar] Technoboy- commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661233845
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -319,7 +323,7 @@ private String getDynamicConfigurationFromStore(String path, String settingName,
private double getDynamicConfigurationDouble(String path, String settingName, double defaultValue) {
double result = defaultValue;
try {
- String setting = this.getDynamicConfigurationFromStore(path, settingName, null);
+ String setting = this.getDynamicConfigurationFromStore(path, settingName, String.valueOf(defaultValue));
Review comment:
Ok, I will split this patch. And pull a new request for test getDynamicConfigurationBoolean.
Thanks for reviewing .
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661432220
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,12 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRankingHandle");
+ final int index = i;
+ Awaitility.await().until(() -> {
+ Object invoke = method.invoke(pulsarServices[index].getLoadManager().get());
Review comment:
you may capture` pulsarServices[index] `out of the loop, instead of capturing `i`
--
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
[GitHub] [pulsar] Technoboy- commented on a change in pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#discussion_r661418013
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java
##########
@@ -321,8 +322,12 @@ public void testBrokerRanking() throws Exception {
}
for (int i = 0; i < BROKER_COUNT; i++) {
- Method updateRanking = Whitebox.getMethod(SimpleLoadManagerImpl.class, "updateRanking");
- updateRanking.invoke(pulsarServices[i].getLoadManager().get());
+ Method method = Whitebox.getMethod(SimpleLoadManagerImpl.class, "getUpdateRankingHandle");
+ Object invoke = method.invoke(pulsarServices[i].getLoadManager().get());
Review comment:
Yes, I will update this. And try to keep the step with community and keep learning from you, from who giving me suggestions and ideas. Thanks very much for reviewing.
--
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
[GitHub] [pulsar] sijie merged pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150
--
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
[GitHub] [pulsar] Technoboy- commented on pull request #11150: Fix Flaky-test: [LoadBalancerTest].[testBrokerRanking]
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #11150:
URL: https://github.com/apache/pulsar/pull/11150#issuecomment-871176140
cc @gaoran10..
--
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