You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "thetumbled (via GitHub)" <gi...@apache.org> on 2023/08/10 09:20:11 UTC
[GitHub] [pulsar] thetumbled opened a new pull request, #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
thetumbled opened a new pull request, #20970:
URL: https://github.com/apache/pulsar/pull/20970
### Motivation
Load balance module in pulsar broker pose greate pressure on zk.
<img width="288" alt="image" src="https://github.com/apache/pulsar/assets/52550727/f85fad25-5d11-4b01-a325-d2d784360413">
<img width="299" alt="image" src="https://github.com/apache/pulsar/assets/52550727/75178de4-d7d9-4522-8af5-b23415560b78">
Currently, `ModularLoadManagerImpl` is the main `LoadManager` in 2.x version, which work as a centralized way. Most of the time, leader broker take the responsibility of shedding and assignning, while followers update their load data to zk for leader. Leader broker retrieve load data of all brokers in the cluster to make decision.
But, i found that not only leader will retrieve all load data (include LocalBrokerData and BundleData), but also followers, which is unnecessary and pose greate pressure to zk.
### Modifications
Disable load data retrieve for followers.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lifepuzzlefun commented on pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "lifepuzzlefun (via GitHub)" <gi...@apache.org>.
lifepuzzlefun commented on PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#issuecomment-1674208162
I support the idea, in large cluster deployment follower read zk is very heavy. We need more unit test on failover or other logic if we remove the retrieve logic.
--
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] lifepuzzlefun commented on a diff in pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "lifepuzzlefun (via GitHub)" <gi...@apache.org>.
lifepuzzlefun commented on code in PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#discussion_r1290889402
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -834,13 +845,30 @@ private void updateBundleSplitMetrics(int bundlesSplit) {
/**
* As the leader broker, find a suitable broker for the assignment of the given bundle.
+ * If leader broker is inactive, decision will be made by the current broker.
*
* @param serviceUnit
* ServiceUnitId for the bundle.
* @return The name of the selected broker, as it appears on metadata store.
*/
@Override
public Optional<String> selectBrokerForAssignment(final ServiceUnitId serviceUnit) {
+ if (oldFlag) {
+ log.info("LoadData is old, need to update loadData before make decision");
+ updateAll();
Review Comment:
does this method protected by locks or oldFlag is less update to true ? I think this change may cause lookup process time much longer, and we need to ensure only one concurrent updateAll() call
--
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 #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 commented on code in PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#discussion_r1295569028
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -275,24 +277,32 @@ public void initialize(final PulsarService pulsar) {
// register listeners for domain changes
pulsar.getPulsarResources().getClusterResources().getFailureDomainResources()
.registerListener(__ -> {
- executors.execute(
+ scheduler.execute(
() -> LoadManagerShared.refreshBrokerToFailureDomainMap(pulsar, brokerToFailureDomainMap));
});
loadSheddingPipeline.add(createLoadSheddingStrategy());
}
+ private boolean isLeader() {
+ return pulsar.getLeaderElectionService() != null
+ && pulsar.getLeaderElectionService().isLeader();
+ }
+
public void handleDataNotification(Notification t) {
if (t.getPath().startsWith(LoadManager.LOADBALANCE_BROKERS_ROOT)) {
brokersData.listLocks(LoadManager.LOADBALANCE_BROKERS_ROOT)
.thenAccept(brokers -> {
reapDeadBrokerPreallocations(brokers);
});
+ oldFlag = true;
- try {
- executors.execute(ModularLoadManagerImpl.this::updateAll);
- } catch (RejectedExecutionException e) {
- // Executor is shutting down
+ if (isLeader()) {
+ try {
+ scheduler.submit(ModularLoadManagerImpl.this::updateAll);
Review Comment:
We need a unit test to verify this change.
--
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] thetumbled commented on pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#issuecomment-1676594431
PTAL, thanks. @codelipenghui @heesung-sn @Demogorgon314
--
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] thetumbled commented on a diff in pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#discussion_r1290940525
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -834,13 +845,30 @@ private void updateBundleSplitMetrics(int bundlesSplit) {
/**
* As the leader broker, find a suitable broker for the assignment of the given bundle.
+ * If leader broker is inactive, decision will be made by the current broker.
*
* @param serviceUnit
* ServiceUnitId for the bundle.
* @return The name of the selected broker, as it appears on metadata store.
*/
@Override
public Optional<String> selectBrokerForAssignment(final ServiceUnitId serviceUnit) {
+ if (oldFlag) {
+ log.info("LoadData is old, need to update loadData before make decision");
+ updateAll();
Review Comment:
updated and ensure that there is only one updateAll() call.
--
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
Re: [PR] [improve] [broker] Follower do not need to retrieve load data in zk. [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
URL: https://github.com/apache/pulsar/pull/20970
--
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] heesung-sn commented on pull request #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#issuecomment-1677701569
> But, i found that not only leader will retrieve all load data (include LocalBrokerData and BundleData), but also followers, which is unnecessary and pose greate pressure to zk.
Can we confirm that followers never use any of this info in their logic(any regressions caused by this change)?
--
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
Re: [PR] [improve] [broker] Follower do not need to retrieve load data in zk. [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#issuecomment-6182285502
@thetumbled thanks for working on this. The PR was left unfinished with the review questions still open: heesung-sohn asked to confirm that followers never use this data, and Demogorgon314 asked for a unit test. ModularLoadManagerImpl has also changed since. We're going through the backlog of open PRs (https://lists.apache.org/thread/cgy7w0qgnq8s7j0ykyzgrn3hj4nb67wf), so I'm closing this one. If you'd like to continue, feel free to reopen it rebased on current master with those points covered.
--
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 #20970: [improve] [broker] Follower do not need to retrieve load data in zk.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20970:
URL: https://github.com/apache/pulsar/pull/20970#issuecomment-1722100001
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