You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "dragosvictor (via GitHub)" <gi...@apache.org> on 2024/01/09 00:00:32 UTC

[PR] [improve][broker] PIP-307: Add feature flag config option [pulsar]

dragosvictor opened a new pull request, #21866:
URL: https://github.com/apache/pulsar/pull/21866

   <!--
   ### Contribution Checklist
     
     - PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*. 
   
     - Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
     
     - Each pull request should address only one issue, not mix up code from multiple issues.
     
     - Each commit in the pull request has a meaningful commit message
   
     - Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
   -->
   
   <!-- If the PR belongs to a PIP, please add the PIP link here -->
   
   PIP: [PIP-307](https://github.com/apache/pulsar/blob/master/pip/pip-307.md)
   
   <!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
   
   ### Motivation
   
   <!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
   
   ### Modifications
   
   <!-- Describe the modifications you've done. -->
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change added tests and can be verified as follows:
   
     - Added unit test `org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImplTest#testOptimizeUnloadDisable` that verifies client lookups occur on both producers and consumers when the flag is set to disabled
     - Manually verified dynamic enablement/disablement of flag in a k8s deployment
   
   ### 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. -->
   - [x] `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: https://github.com/dragosvictor/pulsar/pull/5
   
   


-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "dragosvictor (via GitHub)" <gi...@apache.org>.
dragosvictor commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446833811


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
   Nice, I didn't realize that! Fixing.



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446512404


##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2689,6 +2689,15 @@ The delayed message index time step(in seconds) in per bucket snapshot segment,
     )
     private long loadBalancerServiceUnitStateMonitorIntervalInSeconds = 60;
 
+    @FieldContext(
+            category = CATEGORY_LOAD_BALANCER,
+            dynamic = true,
+            doc = "Enables the fast unloading of bundles. Set to true, forwards destination broker information to "
+                    + "consumers and producers during bundle unload, allowing them to quickly reconnect to the broker "
+                    + "without performing an additional topic lookup."
+    )
+    private boolean loadBalancerOptimizeBundleUnload = true;

Review Comment:
   nit: could we make this config name more elaborative? "Optimize" sounds a little too ambiguous.  
   I think we can rename it to one of the following.
   
   1. loadBalancerMultiPhasePhaseBundleUnload
   2. loadBalancerDelayClientDisconnectionDuringBundleUnload



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -804,7 +804,9 @@ private void handleReleaseEvent(String serviceUnit, ServiceUnitStateData data) {
             if (isTransferCommand(data)) {
                 next = new ServiceUnitStateData(
                         Assigning, data.dstBroker(), data.sourceBroker(), getNextVersionId(data));
-                unloadFuture = closeServiceUnit(serviceUnit, false);
+                // If the optimized bundle unload is disabled, disconnect the clients at time of RELEASE.
+                var disconnectClients = !pulsar.getConfig().isLoadBalancerOptimizeBundleUnload();

Review Comment:
   Don't we need to skip `closeServiceUnit ` at `handleOwnEvent` when isLoadBalancerOptimizeBundleUnload = false?



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446820372


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
    we still need to run this block if `data.force() && isTargetBroker(data.sourceBroker()`, as it's from the orphan cleanup.



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446861124


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,9 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if (isTargetBroker(data.sourceBroker())
+                && (data.force() // orphan cleanup
+                || (isTransferCommand(data) && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()))) { // txfer
             stateChangeListeners.notifyOnCompletion(
                             closeServiceUnit(serviceUnit, true), serviceUnit, data)

Review Comment:
   I am sorry. I realized We still need to log the transfer case even when this flag is disabled.
   



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446827405


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
   I think the logic should be like the following.
   ``` java
   if(
   isTargetBroker(data.sourceBroker())
   && (data.force()  // from orphan cleanup
             || (pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload() && isTransferCommand(data) )) // from transfer
   )
   ```



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446751694


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -804,7 +804,9 @@ private void handleReleaseEvent(String serviceUnit, ServiceUnitStateData data) {
             if (isTransferCommand(data)) {
                 next = new ServiceUnitStateData(
                         Assigning, data.dstBroker(), data.sourceBroker(), getNextVersionId(data));
-                unloadFuture = closeServiceUnit(serviceUnit, false);
+                // If the optimized bundle unload is disabled, disconnect the clients at time of RELEASE.
+                var disconnectClients = !pulsar.getConfig().isLoadBalancerOptimizeBundleUnload();

Review Comment:
   yeah. I agree making this as a non-dynamic is safer to handle such cases.



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446827405


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
   I think the logic should be like the following.
   
   if(
   isTargetBroker(data.sourceBroker())
   && (data.force()  // from orphan cleanup
             || (pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload() && isTransferCommand(data)) // from transfer
   )



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
   I think the logic should be like the following.
   ``` java
   if(
   isTargetBroker(data.sourceBroker())
   && (data.force()  // from orphan cleanup
             || (pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload() && isTransferCommand(data)) // from transfer
   )
   ```



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446861124


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,9 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if (isTargetBroker(data.sourceBroker())
+                && (data.force() // orphan cleanup
+                || (isTransferCommand(data) && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()))) { // txfer
             stateChangeListeners.notifyOnCompletion(
                             closeServiceUnit(serviceUnit, true), serviceUnit, data)

Review Comment:
   I am sorry. I realized We still need to log the transfer case even when this flag is disabled.
   
   I think we need to revert this if-logic, but instead pass the Completed future when this flag is disabled inside the block.



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446820372


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -779,7 +779,8 @@ private void handleOwnEvent(String serviceUnit, ServiceUnitStateData data) {
             lastOwnEventHandledAt = System.currentTimeMillis();
             stateChangeListeners.notify(serviceUnit, data, null);
             log(null, serviceUnit, data, null);
-        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())) {
+        } else if ((data.force() || isTransferCommand(data)) && isTargetBroker(data.sourceBroker())
+                && pulsar.getConfig().isLoadBalancerMultiPhaseBundleUnload()) {

Review Comment:
    we still need to run this block if `data.force() && isTargetBroker(data.sourceBroker()`)



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#issuecomment-1887183742

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: `20 lines` in your changes are missing coverage. Please review.
   > Comparison is base [(`cea5c93`)](https://app.codecov.io/gh/apache/pulsar/commit/cea5c931b43cbc5181aaa79dfee09d53cc1389d1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.59% compared to head [(`f39a066`)](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.59%.
   > Report is 11 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/21866/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #21866      +/-   ##
   ============================================
   - Coverage     73.59%   73.59%   -0.01%     
   - Complexity    32323    32340      +17     
   ============================================
     Files          1858     1859       +1     
     Lines        138174   138291     +117     
     Branches      15148    15161      +13     
   ============================================
   + Hits         101696   101776      +80     
   - Misses        28608    28626      +18     
   - Partials       7870     7889      +19     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21866/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21866/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.12% <4.41%> (-0.13%)` | :arrow_down: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/21866/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.71% <16.17%> (-0.08%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21866/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.88% <70.58%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...org/apache/pulsar/broker/ServiceConfiguration.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvU2VydmljZUNvbmZpZ3VyYXRpb24uamF2YQ==) | `99.39% <100.00%> (+<0.01%)` | :arrow_up: |
   | [...dbalance/extensions/ExtensibleLoadManagerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL0V4dGVuc2libGVMb2FkTWFuYWdlckltcGwuamF2YQ==) | `79.51% <100.00%> (-0.04%)` | :arrow_down: |
   | [...ervice/AbstractDispatcherSingleActiveConsumer.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0RGlzcGF0Y2hlclNpbmdsZUFjdGl2ZUNvbnN1bWVyLmphdmE=) | `90.55% <100.00%> (ø)` | |
   | [...va/org/apache/pulsar/broker/service/ServerCnx.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NlcnZlckNueC5qYXZh) | `72.08% <100.00%> (-0.32%)` | :arrow_down: |
   | [...rg/apache/pulsar/client/impl/TopicListWatcher.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1RvcGljTGlzdFdhdGNoZXIuamF2YQ==) | `67.14% <100.00%> (+0.47%)` | :arrow_up: |
   | [...xtensions/channel/ServiceUnitStateChannelImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZUNoYW5uZWxJbXBsLmphdmE=) | `84.45% <88.88%> (-0.01%)` | :arrow_down: |
   | [...rg/apache/pulsar/broker/service/AbstractTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0VG9waWMuamF2YQ==) | `87.71% <0.00%> (-0.30%)` | :arrow_down: |
   | [...ar/client/impl/PatternMultiTopicsConsumerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21866?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1BhdHRlcm5NdWx0aVRvcGljc0NvbnN1bWVySW1wbC5qYXZh) | `83.45% <63.26%> (-4.51%)` | :arrow_down: |
   
   ... and [80 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21866/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </details>


-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "dragosvictor (via GitHub)" <gi...@apache.org>.
dragosvictor commented on code in PR #21866:
URL: https://github.com/apache/pulsar/pull/21866#discussion_r1446750374


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -804,7 +804,9 @@ private void handleReleaseEvent(String serviceUnit, ServiceUnitStateData data) {
             if (isTransferCommand(data)) {
                 next = new ServiceUnitStateData(
                         Assigning, data.dstBroker(), data.sourceBroker(), getNextVersionId(data));
-                unloadFuture = closeServiceUnit(serviceUnit, false);
+                // If the optimized bundle unload is disabled, disconnect the clients at time of RELEASE.
+                var disconnectClients = !pulsar.getConfig().isLoadBalancerOptimizeBundleUnload();

Review Comment:
   @heesung-sn What would be the consequence of changing this flag in the middle of an operation, between the RELEASE and OWN states? Perhaps I should make it non-dynamic.



-- 
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] PIP-307: Add feature flag config option [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn merged PR #21866:
URL: https://github.com/apache/pulsar/pull/21866


-- 
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