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/07/21 08:15:50 UTC
[GitHub] [pulsar] Demogorgon314 opened a new pull request, #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Demogorgon314 opened a new pull request, #16714:
URL: https://github.com/apache/pulsar/pull/16714
Fixes #16705
### Motivation
When the `getBundleWithHighestThroughput ` method try to use `((ModularLoadManagerWrapper)loadManager).getLoadManager().getBundleDataOrDefault(bundle.toString());` get the bundle load data, the load data might already updated by load manager.
https://github.com/apache/pulsar/blob/4483df2b3d113bfb55a8bb1a9e6c77160257c475/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L1109-L1119
Also, we should give the `bundleWithHighestThroughput` a default value, since is possible to return a null value.
### Modifications
* Disable the `writeResourceQuotasToZooKeeper` task by set load balancer enabled false.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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] codelipenghui commented on a diff in pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#discussion_r926611270
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java:
##########
@@ -602,8 +602,8 @@ public void testSplitLargestBundle() throws Exception {
*/
@Test
public void testSplitBundleWithHighestThroughput() throws Exception {
-
conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName());
+ conf.setLoadBalancerEnabled(false);
Review Comment:
Hmmm, it's a little confusing. The test is related to the load manager but need to disable the load balancer in the test.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java:
##########
@@ -602,8 +602,8 @@ public void testSplitLargestBundle() throws Exception {
*/
@Test
public void testSplitBundleWithHighestThroughput() throws Exception {
-
conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName());
+ conf.setLoadBalancerEnabled(false);
Review Comment:
I think without this change, the flaky test can also be fixed? Since we will not return null for method `getBundleWithHighestThroughput`
--
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] github-actions[bot] commented on pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#issuecomment-1258877367
The pr had no activity for 30 days, mark with Stale label.
--
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] Demogorgon314 commented on a diff in pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#discussion_r926790062
##########
pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java:
##########
@@ -242,11 +242,11 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughput = null;
+ NamespaceBundle bundleWithHighestThroughput = bundles.getBundles().get(0);
Review Comment:
If the bundle don't have any connected topic, the check `bundleData.getTopics() > 0` will never pass, so the `bundleWithHighestThroughput ` is null.
--
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] Demogorgon314 closed pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
Demogorgon314 closed pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
URL: https://github.com/apache/pulsar/pull/16714
--
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] shibd commented on a diff in pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#discussion_r926759023
##########
pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java:
##########
@@ -242,11 +242,11 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughput = null;
+ NamespaceBundle bundleWithHighestThroughput = bundles.getBundles().get(0);
Review Comment:
I don't understand, I think even if the bundle is updated, we should always be able to find the one with the highest load?
--
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] Demogorgon314 commented on a diff in pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#discussion_r926763623
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java:
##########
@@ -602,8 +602,8 @@ public void testSplitLargestBundle() throws Exception {
*/
@Test
public void testSplitBundleWithHighestThroughput() throws Exception {
-
conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName());
+ conf.setLoadBalancerEnabled(false);
Review Comment:
No, disable the load balancer just to cancel the load report task.
You can see the below code, in this test, it tries to write the mock load data to storage, but if the load report task starts, the mock data might be overridden.
https://github.com/apache/pulsar/blob/01349088f86ad8034c844fb1bbe0b0c04e2c0892/pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java#L625-L634
--
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] congbobo184 commented on pull request #16714: [fix][test] NamespaceServiceTest.testSplitBundleWithHighestThroughput
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #16714:
URL: https://github.com/apache/pulsar/pull/16714#issuecomment-1318544070
@Demogorgon314 hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.
--
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