You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "315157973 (via GitHub)" <gi...@apache.org> on 2023/02/18 02:44:47 UTC

[GitHub] [pulsar] 315157973 opened a new pull request, #19559: [fix][broker] Remove useless load items

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

   ### Motivation
   Even for a Broker with a very low load, its memory will grow slowly until GC is triggered. If memory is used as a load calculation item, the Bundle will be unloaded by mistake
   
   ### Modifications
   Remove memory as load calculation item 
   
   ### Verifying this change
   Existing unit tests need to pass all
   
   ### Documentation
   
   - [ ] `doc` <!-- Your PR contains doc changes -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `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] michaeljmarshall commented on a diff in pull request #19559: [fix][broker] Remove useless load balancer items about MemoryResourceWeight

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -168,7 +168,7 @@ private double updateAvgResourceUsage(String broker, LocalBrokerData localBroker
                 brokerAvgResourceUsage.get(broker);
         double resourceUsage = localBrokerData.getMaxResourceUsageWithWeight(
                 conf.getLoadBalancerCPUResourceWeight(),
-                conf.getLoadBalancerMemoryResourceWeight(), conf.getLoadBalancerDirectMemoryResourceWeight(),

Review Comment:
   Nit: it'd be helpful to document the `LoadBalancerMemoryResourceWeight` configuration to show that it is no longer in use and that it will be removed.



##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -249,14 +249,21 @@ public String printResourceUsage() {
                 cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
                 bandwidthOut.percentUsage());
     }
-
+    @Deprecated

Review Comment:
   Nit: what do you think about adding the `since = "3.0.0"` arg to the `@Deprecated` annotation. I did that in a recent PR because it has the benefit of making it clearer when we can remove it later. The history is clear in git, but this lets a user skip a few steps.



-- 
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] michaeljmarshall commented on pull request #19559: [fix][broker] Remove useless load items

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

   > > Please discuss this change on the mailing list. This is a fundamental change.
   > 
   > I don't think it is a fundamental change. This is an obvious bug , memory is not used in the `getMaxResourceUsage` method in the same class. Two methods to get MaxResourceUsage, but the behavior is different
   > 
   > https://github.com/apache/pulsar/blob/660525e57ed35b74cb9204521d1fba02cc08c542/pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java#L239-L243
   > 
   > https://github.com/apache/pulsar/blob/660525e57ed35b74cb9204521d1fba02cc08c542/pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java#L253-L259
   
   Thanks for the references. This justification would have been very helpful to provide in the PR description.
   
   I missed https://github.com/apache/pulsar/pull/17598, which made the inconsistent change that you reference. Before https://github.com/apache/pulsar/pull/17598, we used memory for both methods. That change could certainly have had a note on the mailing list since it is a fundamental change to how the load balancer works.
   
   This change will affect the `LeastResourceUsageWithWeight` and the `ThresholdShedder`.
   
   Interestingly, https://github.com/apache/pulsar/pull/17598 addresses the content of this PR and also addresses @Jason918's comment:
   
   > Note that this PR does not update `getMaxResourceUsageWithWeight*()`, as it already controls the memory usage by `loadBalancerMemoryResourceWeight=0.0`
   
   However, from what I see, `loadBalancerMemoryResourceWeight` defaults to 1, not 0.


-- 
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- closed pull request #19559: [fix][broker] Remove useless load items

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19559: [fix][broker] Remove useless load items
URL: https://github.com/apache/pulsar/pull/19559


-- 
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] codecov-commenter commented on pull request #19559: [fix][broker] Remove useless load items

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

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19559](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3b2c20) into [master](https://codecov.io/gh/apache/pulsar/commit/fe547c7b7e1d4cb344419cbe37dc2ae771ca630f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fe547c7) will **increase** coverage by `33.69%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/19559/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=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #19559       +/-   ##
   =============================================
   + Coverage     28.70%   62.40%   +33.69%     
   - Complexity     6986    25878    +18892     
   =============================================
     Files          1670     1844      +174     
     Lines        125950   135279     +9329     
     Branches      13728    14875     +1147     
   =============================================
   + Hits          36159    84418    +48259     
   + Misses        84392    43103    -41289     
   - Partials       5399     7758     +2359     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | inttests | `24.58% <100.00%> (?)` | |
   | systests | `25.32% <100.00%> (-0.01%)` | :arrow_down: |
   | unittests | `59.47% <100.00%> (+40.00%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...loadbalance/impl/LeastResourceUsageWithWeight.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL0xlYXN0UmVzb3VyY2VVc2FnZVdpdGhXZWlnaHQuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...lsar/broker/loadbalance/impl/ThresholdShedder.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL1RocmVzaG9sZFNoZWRkZXIuamF2YQ==) | `31.14% <100.00%> (+4.09%)` | :arrow_up: |
   | [...ar/policies/data/loadbalancer/LocalBrokerData.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL3BvbGljaWVzL2RhdGEvbG9hZGJhbGFuY2VyL0xvY2FsQnJva2VyRGF0YS5qYXZh) | `92.85% <100.00%> (ø)` | |
   | [...ache/pulsar/zookeeper/LocalBookkeeperEnsemble.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL3pvb2tlZXBlci9Mb2NhbEJvb2trZWVwZXJFbnNlbWJsZS5qYXZh) | `46.50% <0.00%> (-16.09%)` | :arrow_down: |
   | [...r/service/AbstractDispatcherMultipleConsumers.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0RGlzcGF0Y2hlck11bHRpcGxlQ29uc3VtZXJzLmphdmE=) | `70.96% <0.00%> (-9.68%)` | :arrow_down: |
   | [...r/service/schema/DefaultSchemaRegistryService.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3NjaGVtYS9EZWZhdWx0U2NoZW1hUmVnaXN0cnlTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-6.25%)` | :arrow_down: |
   | [...g/apache/pulsar/broker/service/StreamingStats.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N0cmVhbWluZ1N0YXRzLmphdmE=) | `89.18% <0.00%> (-5.41%)` | :arrow_down: |
   | [...in/java/org/apache/pulsar/PulsarBrokerStarter.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL1B1bHNhckJyb2tlclN0YXJ0ZXIuamF2YQ==) | `28.14% <0.00%> (-3.00%)` | :arrow_down: |
   | [...che/pulsar/broker/BookKeeperClientFactoryImpl.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9Cb29rS2VlcGVyQ2xpZW50RmFjdG9yeUltcGwuamF2YQ==) | `50.46% <0.00%> (-2.81%)` | :arrow_down: |
   | [...tion/pendingack/impl/PendingAckHandleDisabled.java](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9wZW5kaW5nYWNrL2ltcGwvUGVuZGluZ0Fja0hhbmRsZURpc2FibGVkLmphdmE=) | `29.41% <0.00%> (-1.84%)` | :arrow_down: |
   | ... and [1255 more](https://codecov.io/gh/apache/pulsar/pull/19559?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
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] michaeljmarshall commented on a diff in pull request #19559: [fix][broker] Remove useless load balancer items about MemoryResourceWeight

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


##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -249,14 +249,21 @@ public String printResourceUsage() {
                 cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
                 bandwidthOut.percentUsage());
     }
-
+    @Deprecated

Review Comment:
   Sorry, I see now that the `since` parameter was introduced in Java 9, and we require `pulsar-common` has Java 8 compatibility. Thanks for trying it out!



-- 
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] 315157973 commented on pull request #19559: [fix][broker] Remove useless load items

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

   > Please discuss this change on the mailing list. This is a fundamental change.
   
   I don't think it is a fundamental change. 
   This is an obvious bug , memory is not used in the `getMaxResourceUsage` method in the same class. 
   Two methods to get MaxResourceUsage, but the behavior is different
   
   https://github.com/apache/pulsar/blob/660525e57ed35b74cb9204521d1fba02cc08c542/pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java#L239-L243
   
   https://github.com/apache/pulsar/blob/660525e57ed35b74cb9204521d1fba02cc08c542/pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java#L253-L259


-- 
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] 315157973 merged pull request #19559: [fix][broker] Remove useless load balancer items about MemoryResourceWeight

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


-- 
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 pull request #19559: [fix][broker] Remove useless load balancer items about MemoryResourceWeight

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

   I agree that we should not remove the methods but deprecated them.
   


-- 
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] michaeljmarshall commented on a diff in pull request #19559: [fix][broker] Remove useless load items

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


##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -250,10 +250,10 @@ public String printResourceUsage() {
                 bandwidthOut.percentUsage());
     }
 
-    public double getMaxResourceUsageWithWeight(final double cpuWeight, final double memoryWeight,
+    public double getMaxResourceUsageWithWeight(final double cpuWeight,
                                                 final double directMemoryWeight, final double bandwidthInWeight,
                                                 final double bandwidthOutWeight) {

Review Comment:
   This is a breaking change to a public method users would likely call if they have custom `LoadSheddingStrategy` implementations. If we move forward removing the memory weight, I think we should first deprecate this method and then remove it later.
   
   Note that we can still change the Pulsar implementations without breaking this method.



-- 
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] 315157973 commented on a diff in pull request #19559: [fix][broker] Remove useless load items

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


##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -250,10 +250,10 @@ public String printResourceUsage() {
                 bandwidthOut.percentUsage());
     }
 
-    public double getMaxResourceUsageWithWeight(final double cpuWeight, final double memoryWeight,
+    public double getMaxResourceUsageWithWeight(final double cpuWeight,
                                                 final double directMemoryWeight, final double bandwidthInWeight,
                                                 final double bandwidthOutWeight) {

Review Comment:
   I will deprecate this method first



-- 
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] 315157973 commented on a diff in pull request #19559: [fix][broker] Remove useless load balancer items about MemoryResourceWeight

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -168,7 +168,7 @@ private double updateAvgResourceUsage(String broker, LocalBrokerData localBroker
                 brokerAvgResourceUsage.get(broker);
         double resourceUsage = localBrokerData.getMaxResourceUsageWithWeight(
                 conf.getLoadBalancerCPUResourceWeight(),
-                conf.getLoadBalancerMemoryResourceWeight(), conf.getLoadBalancerDirectMemoryResourceWeight(),

Review Comment:
   The client module depends on the pulsar-common module, so the language level is still at 1.8, and we can not set `since` of Deprecated



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