[PIP-82] [pulsar-broker] incorporate review feedback - #10201
Conversation
| ) | ||
| private String resourceUsageTransportClassName = ""; | ||
|
|
||
| @FieldContext( |
There was a problem hiding this comment.
What was the rationale for making this not configurable?
There was a problem hiding this comment.
@jerrypeng while reviewing the previous PR(#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.
| private final Map<String, ResourceUsageConsumer> | ||
| consumerMap = new ConcurrentHashMap<String, ResourceUsageConsumer>(); | ||
|
|
||
| private long staleMessageCount = 0; |
There was a problem hiding this comment.
This variable doesn't seem to be used anywhere?
There was a problem hiding this 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.
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
5 similar comments
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
- 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.
1176fde to
b4cac86
Compare
|
/pulsarbot run-failure-checks |
2 similar comments
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
@jerrypeng CICD seems broken for this merge |
|
|
|
Seems should remove this line Same in |
This reverts commit 1cc782f.
|
@merlimat PTAL |
- possibly a merge conflict in apache#10201
|
@linlinnn @jerrypeng @codelipenghui @eolivelli @merlimat @aahmed-se I created #10250 to fix the compilation issues in master branch. |
- possibly a merge conflict in #10201
| if (!tenantList.contains(tenant)) { | ||
| admin.tenants().createTenant(tenant, | ||
| new TenantInfo(Sets.newHashSet(config.getSuperUserRoles()), Sets.newHashSet(cluster))); | ||
| try { |
There was a problem hiding this comment.
To add try/catch is a good solution.
7 participants
Motivation
Follow up and incorporate the review feedback.
Modifications
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.