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/11/24 06:31:42 UTC
[GitHub] [pulsar] gaozhangmin opened a new pull request #12957: fix pr #12378
gaozhangmin opened a new pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957
### Motivation
Pr #12378 Provided an option to split bundle based on load, there are 2 problems:
1、Method `getBundleWithHighestThroughput` returned a bundle which not owned by any broker, split would be failed.
```
11:46:23.080 [AsyncHttpClient-7-1] WARN org.apache.pulsar.client.admin.internal.BaseResource - [http://localhost:8080/admin/v2/namespaces/public/default/HOT/split?unload=false] Failed to perform http put request: javax.ws.rs.ClientErrorException: HTTP 412 Precondition Failed
Failed to find ownership for ServiceUnit:public/default/0x00000000_0x10000000
Reason: Failed to find ownership for ServiceUnit:public/default/0x00000000_0x10000000
```
2、incorrect bundle-data path.
### Modifications
correct the above errors
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [ ] `doc-required`
(If you need help on updating docs, create a doc issue)
- [ ] `no-need-doc`
(Please explain why)
- [ ] `doc`
(If this PR contains doc changes)
--
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] gaozhangmin commented on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-979612792
/pulsarbot run-failure-checks
--
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 #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-977573086
@gaozhangmin:Thanks for providing doc info!
--
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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763722847
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
there are two situation that getBundleWithHighestThroughput would return a bundle which is owned by none of broker.
1、no topics existed in namespace.
2、msgThroughput of topics is smaller than default value 50000.
--
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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763821108
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
I don't think so,
`bundleData.getTopics() > 0` condition makes sure that this bundle is owned by broker. which is precondition of `split-bundle`
--
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] gaozhangmin commented on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-977573936
@lhotari @codelipenghui @BewareMyPower PTAL
--
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 a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763815616
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
I think the correct modification should not use the default value, should use the real value, and then deal with the null situation
--
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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763720206
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
there is a default value 50000. this situation may exists.
--
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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r756606778
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java
##########
@@ -570,13 +570,15 @@ public void testSplitBundleWithHighestThroughput() throws Exception {
NamespaceBundles bundles = namespaceService.getNamespaceBundleFactory().getBundles(nsname);
String bundle = bundles.findBundle(TopicName.get(topic + "0")).getBundleRange();
- String path = ModularLoadManagerImpl.getBundleDataPath(bundle);
+ String path = ModularLoadManagerImpl.getBundleDataPath(namespace + "/" + bundle);
NamespaceBundleStats defaultStats = new NamespaceBundleStats();
defaultStats.msgThroughputIn = 100000;
defaultStats.msgThroughputOut = 100000;
- BundleData bd = new BundleData(10, 19, defaultStats );
+ BundleData bd = new BundleData(10, 19, defaultStats);
+ bd.setTopics(10);
byte[] data = ObjectMapperFactory.getThreadLocal().writeValueAsBytes(bd);
pulsar.getLocalMetadataStore().put(path, data, Optional.empty());
+ Thread.sleep(3);
Review comment:
PTAL
--
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] lhotari commented on a change in pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r786549998
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundle.java
##########
@@ -124,7 +124,7 @@ public String getBundleRange() {
return bundleRange;
}
- private static String getKey(NamespaceName nsname, Range<Long> keyRange) {
+ public static String getKey(NamespaceName nsname, Range<Long> keyRange) {
return String.format("%s/0x%08x_0x%08x", nsname, keyRange.lowerEndpoint(), keyRange.upperEndpoint());
Review comment:
The performance of String.format is bad and it causes a lot of allocations. It's fine to use it when there aren't a large number of invocations. That was optimized for NamespaceBundle in #9976.
--
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] lhotari commented on a change in pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r786548383
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundle.java
##########
@@ -124,7 +124,7 @@ public String getBundleRange() {
return bundleRange;
}
- private static String getKey(NamespaceName nsname, Range<Long> keyRange) {
+ public static String getKey(NamespaceName nsname, Range<Long> keyRange) {
return String.format("%s/0x%08x_0x%08x", nsname, keyRange.lowerEndpoint(), keyRange.upperEndpoint());
Review comment:
Please don't expose this method because of performance reasons. Instead, add an instance method "getKey()" to NamespaceBundle so that the cached key can be retrieved.
NamespaceBundle.toString() already returns the cached key, so you could also use that instead of adding a new getKey method.
That seems to be your intention to get the key for the NamespaceBundle?
--
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] gaozhangmin commented on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1005555599
@eolivelli PTAL
--
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] gaozhangmin commented on pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1032559151
@lhotari Please help review the PR again.
--
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 a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763849900
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
if we use the real value, we don't need to check bundleData.getTopics() > 0 right? The default value make the logical very unfriendly
--
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] gaozhangmin commented on pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1015376848
/pulsarbot run-failure-checks
--
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 pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1018164349
@lhotari Please help review the PR again.
--
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] gaozhangmin commented on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-977591743
@rdhabalia PTAL
--
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 change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r757402295
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java
##########
@@ -570,13 +570,15 @@ public void testSplitBundleWithHighestThroughput() throws Exception {
NamespaceBundles bundles = namespaceService.getNamespaceBundleFactory().getBundles(nsname);
String bundle = bundles.findBundle(TopicName.get(topic + "0")).getBundleRange();
- String path = ModularLoadManagerImpl.getBundleDataPath(bundle);
+ String path = ModularLoadManagerImpl.getBundleDataPath(namespace + "/" + bundle);
NamespaceBundleStats defaultStats = new NamespaceBundleStats();
defaultStats.msgThroughputIn = 100000;
defaultStats.msgThroughputOut = 100000;
- BundleData bd = new BundleData(10, 19, defaultStats );
+ BundleData bd = new BundleData(10, 19, defaultStats);
+ bd.setTopics(10);
byte[] data = ObjectMapperFactory.getThreadLocal().writeValueAsBytes(bd);
pulsar.getLocalMetadataStore().put(path, data, Optional.empty());
+ Thread.sleep(3);
Review comment:
@gaozhangmin Could you please point out why the getBundleWithHighestThroughput() returns a wrong bundle name? which from your PR description.
--
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] lhotari commented on a change in pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r786548383
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundle.java
##########
@@ -124,7 +124,7 @@ public String getBundleRange() {
return bundleRange;
}
- private static String getKey(NamespaceName nsname, Range<Long> keyRange) {
+ public static String getKey(NamespaceName nsname, Range<Long> keyRange) {
return String.format("%s/0x%08x_0x%08x", nsname, keyRange.lowerEndpoint(), keyRange.upperEndpoint());
Review comment:
Please don't expose this method because of performance reasons. Instead, add an instance method "getKey()" to NamespaceBundle so that the cached key can be retrieved. That seems to be your intention to get the key for the NamespaceBundle?
--
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] lhotari merged pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957
--
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 a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763717783
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
why `bundleData.getTopics()` <= 0 then bundleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput? This situation shouldn't exist, 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] congbobo184 commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763815616
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
I think the correct modification should not use the default value, but use the real value, and then deal with the null situation
--
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] gaozhangmin edited a comment on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin edited a comment on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-977591743
@rdhabalia @eolivelli PTAL
--
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] gaozhangmin commented on pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-978952524
/pulsarbot run-failure-checks
--
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 change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r756572171
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java
##########
@@ -570,13 +570,15 @@ public void testSplitBundleWithHighestThroughput() throws Exception {
NamespaceBundles bundles = namespaceService.getNamespaceBundleFactory().getBundles(nsname);
String bundle = bundles.findBundle(TopicName.get(topic + "0")).getBundleRange();
- String path = ModularLoadManagerImpl.getBundleDataPath(bundle);
+ String path = ModularLoadManagerImpl.getBundleDataPath(namespace + "/" + bundle);
NamespaceBundleStats defaultStats = new NamespaceBundleStats();
defaultStats.msgThroughputIn = 100000;
defaultStats.msgThroughputOut = 100000;
- BundleData bd = new BundleData(10, 19, defaultStats );
+ BundleData bd = new BundleData(10, 19, defaultStats);
+ bd.setTopics(10);
byte[] data = ObjectMapperFactory.getThreadLocal().writeValueAsBytes(bd);
pulsar.getLocalMetadataStore().put(path, data, Optional.empty());
+ Thread.sleep(3);
Review comment:
Can you try with the Awaitibility?
--
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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r757416667
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceServiceTest.java
##########
@@ -570,13 +570,15 @@ public void testSplitBundleWithHighestThroughput() throws Exception {
NamespaceBundles bundles = namespaceService.getNamespaceBundleFactory().getBundles(nsname);
String bundle = bundles.findBundle(TopicName.get(topic + "0")).getBundleRange();
- String path = ModularLoadManagerImpl.getBundleDataPath(bundle);
+ String path = ModularLoadManagerImpl.getBundleDataPath(namespace + "/" + bundle);
NamespaceBundleStats defaultStats = new NamespaceBundleStats();
defaultStats.msgThroughputIn = 100000;
defaultStats.msgThroughputOut = 100000;
- BundleData bd = new BundleData(10, 19, defaultStats );
+ BundleData bd = new BundleData(10, 19, defaultStats);
+ bd.setTopics(10);
byte[] data = ObjectMapperFactory.getThreadLocal().writeValueAsBytes(bd);
pulsar.getLocalMetadataStore().put(path, data, Optional.empty());
+ Thread.sleep(3);
Review comment:
1、 Incorrect bundle path, when getting bundle-data from zk. missed namespace info.
2、Split an unowned bundle is not allowed. which getBundleWithHighestThroughput would return.
--
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] gaozhangmin commented on pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1015441296
/pulsarbot run-failure-checks
--
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] gaozhangmin commented on pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-1016028216
/pulsarbot run-failure-checks
--
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] gaozhangmin commented on a change in pull request #12957: Failed split bundle base on throughput
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r786579144
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundle.java
##########
@@ -124,7 +124,7 @@ public String getBundleRange() {
return bundleRange;
}
- private static String getKey(NamespaceName nsname, Range<Long> keyRange) {
+ public static String getKey(NamespaceName nsname, Range<Long> keyRange) {
return String.format("%s/0x%08x_0x%08x", nsname, keyRange.lowerEndpoint(), keyRange.upperEndpoint());
Review comment:
@lhotari Thanks for your correction. PTAL again.
--
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 #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#issuecomment-977572952
@gaozhangmin:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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
[GitHub] [pulsar] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r764512908
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
For unassigned bundle ,there is no totalMsgThroughput, using default value 50000, We cannot tell from default from real value ,if real value is also 50000.
--
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 a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763717783
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
why `bundleData.getTopics()` < 0 then bundleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput? This situation shouldn't exist, 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] gaozhangmin commented on a change in pull request #12957: fix pr #12378
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #12957:
URL: https://github.com/apache/pulsar/pull/12957#discussion_r763720206
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -212,16 +212,18 @@ public NamespaceBundle getBundleWithHighestThroughput(NamespaceName nsName) {
if (loadManager instanceof ModularLoadManagerWrapper) {
NamespaceBundles bundles = getBundles(nsName);
double maxMsgThroughput = -1;
- NamespaceBundle bundleWithHighestThroughpit = null;
+ NamespaceBundle bundleWithHighestThroughput = null;
for (NamespaceBundle bundle : bundles.getBundles()) {
- BundleData budnleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
- .getBundleDataOrDefault(bundle.getBundleRange());
- if (budnleData.getLongTermData().totalMsgThroughput() > maxMsgThroughput) {
- maxMsgThroughput = budnleData.getLongTermData().totalMsgThroughput();
- bundleWithHighestThroughpit = bundle;
+ BundleData bundleData = ((ModularLoadManagerWrapper) loadManager).getLoadManager()
+ .getBundleDataOrDefault(NamespaceBundle.getKey(bundle.getNamespaceObject(),
+ bundle.getKeyRange()));
+ if (bundleData.getTopics() > 0
Review comment:
there is a default value 50000. this situation may exist.
--
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