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 2021/04/12 20:57:17 UTC
[GitHub] [pulsar] bharanic-dev opened a new pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
bharanic-dev opened a new pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201
### Motivation
Follow up and incorporate the review feedback.
### Modifications
- Have lightproto generate sources to a different directory so it doesn't come in the way of protobuf generated sources.
- make resourceUsageTransportPublishTopicName internal(remove from config)
- drop old/stale resource-usage messages
- fix a race condition in creating tenant/namespace for resource-usage when multiple brokers attempt to do it.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn edited a comment on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn edited a comment on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821760369
forget remove this [line](https://github.com/apache/pulsar/blob/1cc782f88b8934e1c636d45d9f6fc25311ebe260/pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupServiceTest.java#L254) or add the relative property
` this.conf.setResourceUsageTransportPublishTopicName(INTERNAL_TOPIC);`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-819902486
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-820759589
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#discussion_r612016610
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -687,12 +687,6 @@
)
private String resourceUsageTransportClassName = "";
- @FieldContext(
Review comment:
What was the rationale for making this not configurable?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-819592509
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821759638
@jerrypeng CICD seems broken for this merge
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-818803670
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-819949918
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-818881506
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821759712
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-820621169
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821770427
@linlinnn @jerrypeng @codelipenghui @eolivelli @merlimat @aahmed-se I created #10250 to fix the compilation issues in master branch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#discussion_r612018728
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java
##########
@@ -159,9 +171,11 @@ public void received(Reader<byte[]> reader, Message<byte[]> msg) {
private final Map<String, ResourceUsageConsumer>
consumerMap = new ConcurrentHashMap<String, ResourceUsageConsumer>();
+ private long staleMessageCount = 0;
Review comment:
This variable doesn't seem to be used anywhere?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821760369
forget remove this line
` this.conf.setResourceUsageTransportPublishTopicName(INTERNAL_TOPIC);`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on a change in pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on a change in pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#discussion_r612024955
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java
##########
@@ -159,9 +171,11 @@ public void received(Reader<byte[]> reader, Message<byte[]> msg) {
private final Map<String, ResourceUsageConsumer>
consumerMap = new ConcurrentHashMap<String, ResourceUsageConsumer>();
+ private long staleMessageCount = 0;
Review comment:
I added the variable with the intention of adding a unit test for it. But could not figure out a easy way to do so. I am open to ideas if you have suggestions on how I can add one. Else, I will remove this variable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn edited a comment on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn edited a comment on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821760369
should remove this [line](https://github.com/apache/pulsar/blob/1cc782f88b8934e1c636d45d9f6fc25311ebe260/pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupServiceTest.java#L254)
` this.conf.setResourceUsageTransportPublishTopicName(INTERNAL_TOPIC);`
but actually remove `this.conf.setResourceUsageTransportClassName("org.apache.pulsar.broker.resourcegroup.ResourceUsageTransportManager");`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng merged pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
jerrypeng merged pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn edited a comment on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn edited a comment on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821760369
forget remove this [line](https://github.com/apache/pulsar/blob/1cc782f88b8934e1c636d45d9f6fc25311ebe260/pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupServiceTest.java#L254)
` this.conf.setResourceUsageTransportPublishTopicName(INTERNAL_TOPIC);`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on a change in pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on a change in pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#discussion_r612024612
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -687,12 +687,6 @@
)
private String resourceUsageTransportClassName = "";
- @FieldContext(
Review comment:
@jerrypeng while reviewing the previous PR(https://github.com/apache/pulsar/pull/10008), @codelipenghui and @315157973 gave feedback that the topic name is internal implementation detail that the user doesn't need to be exposed to. I did not have a strong reason to go the other way, so made this change. We can always make it configurable in the future, if we find that there is a use-case for it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-820531050
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-820700779
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-819802726
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn edited a comment on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn edited a comment on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821760369
Seems should remove this [line](https://github.com/apache/pulsar/blob/1cc782f88b8934e1c636d45d9f6fc25311ebe260/pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupServiceTest.java#L254)
` this.conf.setResourceUsageTransportPublishTopicName(INTERNAL_TOPIC);`
but actually remove `this.conf.setResourceUsageTransportClassName("org.apache.pulsar.broker.resourcegroup.ResourceUsageTransportManager");`
Same in `ResourceGroupUsageAggregationTest`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] linlinnn commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-821764293
@merlimat PTAL
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10201: [PIP-82] [pulsar-broker] incorporate review feedback
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10201:
URL: https://github.com/apache/pulsar/pull/10201#issuecomment-819088308
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org