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 06:38:16 UTC

[GitHub] [pulsar] 315157973 opened a new pull request, #19560: [fix][broker] Fixed history load not releasing

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

   ### Motivation
   When a broker in the cluster is not active, its load still remain in the memory of the Leader. 
   If deployed in a container, brokers will scale up and down frequently, and the memory of the Leader gradually increase
   
   ### Modifications
   When a broker is no longer active, delete its load information
   
   ### Verifying this change
   
   ### 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] Demogorgon314 commented on a diff in pull request #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   Ok, in my opinion, `findBundlesForUnloading` method only calls in the leader on one thread. The default Load shedding interval is 1 minute, so it should not be a hot path.
   
   But the current solution is also ok with me.



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }

Review Comment:
   If we modify `brokerAvgResourceUsage` directly, the entire `ThresholdShedder` needs to be locked every time, because there are 2 threads modify `brokerAvgResourceUsage` at the same time.
   And now we don't need to lock every time, only when broker 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] 315157973 commented on pull request #19560: [fix][broker] Fixed history load not releasing

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

   > I see brokerAvgResourceUsage is not a thread safe container, are all accesses to brokerAvgResourceUsage in the same thread?
   
   There is a single-threaded scheduler in Leader Broker to execute him, so it is thread-safe
   
   > What about using a AtomicReference instead of using a concurrent map?
   
   What we need to ensure is the thread safety of activeBrokers, because there will be two threads operating it at the same time. We need to ensure the atomicity of its `read` and `clear`
   The operation of `brokerAvgResourceUsage` is thread-safe and runs in a single thread
   
   


-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -57,10 +60,12 @@ public class ThresholdShedder implements LoadSheddingStrategy {
 
     private static final double MB = 1024 * 1024;
     private final Map<String, Double> brokerAvgResourceUsage = new HashMap<>();
+    private final Set<String> activeBrokers = Collections.newSetFromMap(new ConcurrentHashMap<>());

Review Comment:
   Why use a thread-safe set here? I see you already used `synchronized`.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   Also, we can use `keySet().retainAll(activeBrokers)` to get Intersection.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   Can we clean this map in the `onBrokerChange` 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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -57,10 +60,12 @@ public class ThresholdShedder implements LoadSheddingStrategy {
 
     private static final double MB = 1024 * 1024;
     private final Map<String, Double> brokerAvgResourceUsage = new HashMap<>();
+    private final Set<String> activeBrokers = Collections.newSetFromMap(new ConcurrentHashMap<>());

Review Comment:
   Not always lock



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   I see. Can we change the `brokerAvgResourceUsage` to `ConcurrentHashMap`, and remove the inactive broker in the `onBrokerChange` method?
   
   ```
   @Override
   public void onBrokerChange(Set<String> newBrokers) {
         brokerAvgResourceUsage.keySet().retainAll(newBrokers);
   }
   
   ```



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));
+                    activeBrokers.clear();

Review Comment:
   We don't want to clean up the inactive Broker every time, because it needs to be locked. Therefore, we need an indicator to determine whether to clean up



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui commented on a diff in pull request #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }

Review Comment:
   Why not update the `brokerAvgResourceUsage` without maintaining a new `activeBrokers` map?



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LoadSheddingStrategy.java:
##########
@@ -36,4 +37,11 @@ public interface LoadSheddingStrategy {
      * @return A map from all selected bundles to the brokers on which they reside.
      */
     Multimap<String, String> findBundlesForUnloading(LoadData loadData, ServiceConfiguration conf);
+
+    /**
+     * Triggered when active broker changes.
+     *
+     * @param activeBrokers active Brokers
+     */
+    default void onBrokerChange(Set<String> activeBrokers) {}

Review Comment:
   ```suggestion
       default void onActiveBrokersChange(Set<String> activeBrokers) {}
   ```



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LoadSheddingStrategy.java:
##########
@@ -36,4 +37,11 @@ public interface LoadSheddingStrategy {
      * @return A map from all selected bundles to the brokers on which they reside.
      */
     Multimap<String, String> findBundlesForUnloading(LoadData loadData, ServiceConfiguration conf);
+
+    /**
+     * Triggered when active broker changes.
+     *
+     * @param activeBrokers active Brokers
+     */
+    default void onBrokerChange(Set<String> activeBrokers) {}

Review Comment:
   Is it better to introduce an event listener interface to the LoadManager? Not all load shedding strategies are required the broker's change event. So who cares about the event, then add the listener to the load manager.



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   > Can we clean this map in the onBrokerChange method?
   
   If we clean the map in `onBrokerChange`，we have to lock every time, but now we don't need to lock every time
   
   > Also, we can use keySet().retainAll(activeBrokers) to get Intersection.
   
   We don't want to clean up the inactive Broker every time, because it needs to be locked. 
   If we don't clear this collection, we have to lock it every time to judge whether it needs to be cleaned up



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


-- 
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 #19560: [fix][broker] Fixed history load not releasing

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19560: [fix][broker] Fixed history load not releasing
URL: https://github.com/apache/pulsar/pull/19560


-- 
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 #19560: [fix][broker] Fixed history load not releasing

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

   /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] lifepuzzlefun commented on a diff in pull request #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {

Review Comment:
   can this method be called only when necessary ? 



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   I see. Can we change the `brokerAvgResourceUsage` to `ConcurrentHashMap`, and remove the inactive broker in the `onBrokerChange` method?
   
   ```
   @Override
       public void onBrokerChange(Set<String> newBrokers) {
             brokerAvgResourceUsage.keySet().retainAll(newBrokers);
       }
   
   ```



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));

Review Comment:
   In this way, the place where `brokerAvgResourceUsage` is used in `findBundlesForUnloading` must be locked, that is, it must be locked every time, because now `brokerAvgResourceUsage` will be called in the `updateAvgResourceUsage` method every time. 



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {

Review Comment:
   can this method be called only when necessary ? 



-- 
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- commented on a diff in pull request #19560: [fix][broker] Fixed history load not releasing

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java:
##########
@@ -233,5 +237,23 @@ private Pair<Boolean, String> getMaxUsageBroker(
         }
         return Pair.of(hasBrokerBelowLowerBound, maxUsageBrokerName);
     }
+    @Override
+    public void onBrokerChange(Set<String> newBrokers) {
+        synchronized (activeBrokers) {
+            activeBrokers.clear();
+            activeBrokers.addAll(newBrokers);
+        }
+    }
+
+    private void cleanUnActiveBroker() {
+        if (!activeBrokers.isEmpty()) {
+            synchronized (activeBrokers) {
+                if (!activeBrokers.isEmpty()) {
+                    brokerAvgResourceUsage.keySet().removeIf((key) -> !activeBrokers.contains(key));
+                    activeBrokers.clear();

Review Comment:
   Why clear `activeBrokers` ?



-- 
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 #19560: [fix][broker] Fixed history load not releasing

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

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/19560?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 [#19560](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aef9542) into [master](https://codecov.io/gh/apache/pulsar/commit/153e4d4cc3b56aaee224b0a68e0186c08125c975?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (153e4d4) will **increase** coverage by `29.65%`.
   > The diff coverage is `88.88%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/19560/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/19560?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   #19560       +/-   ##
   =============================================
   + Coverage     32.26%   61.92%   +29.65%     
   + Complexity     7073     3400     -3673     
   =============================================
     Files          1670     1845      +175     
     Lines        125949   135541     +9592     
     Branches      13729    14918     +1189     
   =============================================
   + Hits          40643    83934    +43291     
   + Misses        79356    43860    -35496     
   - Partials       5950     7747     +1797     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | inttests | `24.59% <55.55%> (-0.08%)` | :arrow_down: |
   | systests | `25.21% <44.44%> (-0.12%)` | :arrow_down: |
   | unittests | `59.16% <88.88%> (+39.70%)` | :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/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ulsar/broker/loadbalance/LoadSheddingStrategy.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9Mb2FkU2hlZGRpbmdTdHJhdGVneS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...apache/pulsar/client/impl/ConsumerBuilderImpl.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVyQnVpbGRlckltcGwuamF2YQ==) | `78.37% <ø> (+45.78%)` | :arrow_up: |
   | [...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRMZWRnZXJJbXBsLmphdmE=) | `79.01% <100.00%> (+33.88%)` | :arrow_up: |
   | [...roker/loadbalance/impl/ModularLoadManagerImpl.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL01vZHVsYXJMb2FkTWFuYWdlckltcGwuamF2YQ==) | `66.83% <100.00%> (+5.38%)` | :arrow_up: |
   | [...lsar/broker/loadbalance/impl/ThresholdShedder.java](https://codecov.io/gh/apache/pulsar/pull/19560?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==) | `32.25% <100.00%> (+1.11%)` | :arrow_up: |
   | [...rg/apache/pulsar/io/kafka/KafkaAbstractSource.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWlvL2thZmthL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvaW8va2Fma2EvS2Fma2FBYnN0cmFjdFNvdXJjZS5qYXZh) | `68.88% <100.00%> (+68.88%)` | :arrow_up: |
   | [...va/org/apache/pulsar/common/util/NumberFormat.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi91dGlsL051bWJlckZvcm1hdC5qYXZh) | `66.66% <0.00%> (-12.50%)` | :arrow_down: |
   | [...g/apache/pulsar/broker/service/StreamingStats.java](https://codecov.io/gh/apache/pulsar/pull/19560?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: |
   | [...ar/functions/source/MultiConsumerPulsarSource.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy9pbnN0YW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Z1bmN0aW9ucy9zb3VyY2UvTXVsdGlDb25zdW1lclB1bHNhclNvdXJjZS5qYXZh) | `89.47% <0.00%> (-2.20%)` | :arrow_down: |
   | [...ns/channel/ServiceUnitStateCompactionStrategy.java](https://codecov.io/gh/apache/pulsar/pull/19560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZUNvbXBhY3Rpb25TdHJhdGVneS5qYXZh) | `11.42% <0.00%> (-1.91%)` | :arrow_down: |
   | ... and [1216 more](https://codecov.io/gh/apache/pulsar/pull/19560?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