You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/08 09:14:31 UTC

[GitHub] [pulsar] mattisonchao opened a new pull request, #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

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

   ### Motivation
   
   This break was introduced by #14648 and affected unreleased 2.11.
   
   ### Modifications
   
   - Multiply `overrideBrokerNicSpeedGbps` with the NIC number.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI 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] mattisonchao closed pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`
URL: https://github.com/apache/pulsar/pull/18818


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

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

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

[GitHub] [pulsar] congbobo184 commented on a diff in pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043202387


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java:
##########
@@ -121,10 +122,12 @@ public void calculateBrokerHostUsage() {
         this.usage = usage;
     }
 
-    private double getTotalNicLimitWithConfiguration(List<String> nics) {
+    @VisibleForTesting
+    public double getTotalNicLimitWithConfiguration(List<String> nics) {

Review Comment:
   it's a public method, so can delete `@VisibleForTesting`



-- 
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] mattisonchao commented on a diff in pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043222430


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java:
##########
@@ -121,10 +122,12 @@ public void calculateBrokerHostUsage() {
         this.usage = usage;
     }
 
-    private double getTotalNicLimitWithConfiguration(List<String> nics) {
+    @VisibleForTesting
+    public double getTotalNicLimitWithConfiguration(List<String> nics) {

Review Comment:
   change to protected



-- 
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] mattisonchao commented on a diff in pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043206838


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java:
##########
@@ -121,10 +122,12 @@ public void calculateBrokerHostUsage() {
         this.usage = usage;
     }
 
-    private double getTotalNicLimitWithConfiguration(List<String> nics) {
+    @VisibleForTesting
+    public double getTotalNicLimitWithConfiguration(List<String> nics) {

Review Comment:
   `@VisibleForTesting ` is used to notify this method public for testing. If I change it to private the test can not work anymore.



-- 
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 #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043109513


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LinuxBrokerHostUsageImplTest.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.loadbalance;
+
+import org.apache.pulsar.broker.loadbalance.impl.LinuxBrokerHostUsageImpl;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+
+public class LinuxBrokerHostUsageImplTest {
+
+    @Test
+    public void checkOverrideBrokerNicSpeedGbps() {
+        ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();

Review Comment:
   Close this executor



-- 
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] mattisonchao commented on a diff in pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043112846


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LinuxBrokerHostUsageImplTest.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.loadbalance;
+
+import org.apache.pulsar.broker.loadbalance.impl.LinuxBrokerHostUsageImpl;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+
+public class LinuxBrokerHostUsageImplTest {
+
+    @Test
+    public void checkOverrideBrokerNicSpeedGbps() {
+        ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();

Review Comment:
   fixed



-- 
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 #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#issuecomment-1342369131

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/18818?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 [#18818](https://codecov.io/gh/apache/pulsar/pull/18818?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2034174) into [master](https://codecov.io/gh/apache/pulsar/commit/68ca60cea7665e6a4bd9f07518b038c17893fe02?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (68ca60c) will **decrease** coverage by `12.98%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/18818/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/18818?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   #18818       +/-   ##
   =============================================
   - Coverage     50.05%   37.07%   -12.99%     
   + Complexity    11024     1970     -9054     
   =============================================
     Files           703      209      -494     
     Lines         68814    14425    -54389     
     Branches       7378     1574     -5804     
   =============================================
   - Hits          34446     5348    -29098     
   + Misses        30621     8491    -22130     
   + Partials       3747      586     -3161     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `37.07% <ø> (-12.99%)` | :arrow_down: |
   
   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/18818?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/pulsar/client/impl/PulsarClientImpl.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1B1bHNhckNsaWVudEltcGwuamF2YQ==) | `43.47% <0.00%> (-1.33%)` | :arrow_down: |
   | [...ker/loadbalance/impl/LinuxBrokerHostUsageImpl.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL0xpbnV4QnJva2VySG9zdFVzYWdlSW1wbC5qYXZh) | | |
   | [...ersistentStickyKeyDispatcherMultipleConsumers.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFN0aWNreUtleURpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVycy5qYXZh) | | |
   | [...r/stats/prometheus/PrometheusMetricsGenerator.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9wcm9tZXRoZXVzL1Byb21ldGhldXNNZXRyaWNzR2VuZXJhdG9yLmphdmE=) | | |
   | [...ker/service/schema/exceptions/SchemaException.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3NjaGVtYS9leGNlcHRpb25zL1NjaGVtYUV4Y2VwdGlvbi5qYXZh) | | |
   | [...eper/mledger/offload/jcloud/OffloadIndexBlock.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvT2ZmbG9hZEluZGV4QmxvY2suamF2YQ==) | | |
   | [...on/buffer/impl/TopicTransactionBufferProvider.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9idWZmZXIvaW1wbC9Ub3BpY1RyYW5zYWN0aW9uQnVmZmVyUHJvdmlkZXIuamF2YQ==) | | |
   | [...rg/apache/pulsar/broker/web/PulsarWebResource.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci93ZWIvUHVsc2FyV2ViUmVzb3VyY2UuamF2YQ==) | | |
   | [...n/coordinator/TransactionCoordinatorException.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9leGNlcHRpb24vY29vcmRpbmF0b3IvVHJhbnNhY3Rpb25Db29yZGluYXRvckV4Y2VwdGlvbi5qYXZh) | | |
   | [...er/loadbalance/impl/PulsarResourceDescription.java](https://codecov.io/gh/apache/pulsar/pull/18818/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL1B1bHNhclJlc291cmNlRGVzY3JpcHRpb24uamF2YQ==) | | |
   | ... and [486 more](https://codecov.io/gh/apache/pulsar/pull/18818/diff?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] mattisonchao commented on a diff in pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#discussion_r1043206838


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java:
##########
@@ -121,10 +122,12 @@ public void calculateBrokerHostUsage() {
         this.usage = usage;
     }
 
-    private double getTotalNicLimitWithConfiguration(List<String> nics) {
+    @VisibleForTesting
+    public double getTotalNicLimitWithConfiguration(List<String> nics) {

Review Comment:
   `@VisibleForTesting ` is used to notify this method public for testing. If I change it to private the test can not work anymore.



-- 
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- merged pull request #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #18818:
URL: https://github.com/apache/pulsar/pull/18818


-- 
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 #18818: [fix][broker] Fix the wrong behaviour when set `overrideBrokerNicSpeedGbps`

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18818:
URL: https://github.com/apache/pulsar/pull/18818#issuecomment-1342319402

   @mattisonchao Please add the following content to your PR description and select a checkbox:
   ```
   - [ ] `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 -->
   ```


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