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/05/19 22:20:39 UTC
[GitHub] [pulsar] dlg99 opened a new pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
dlg99 opened a new pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #10611
### Motivation
Stats report wrong values when consumers unsubscribe.
### Modifications
Preserved specific stats for the consumer on its removal.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change added tests and can be verified as follows:
- Added unit tests for persistent and non-persistent topics to check that specific counters preserved after consumers/producers removal. Tests failed before the fix.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (NO
- The public API: NO
- The schema: NO
- The default values of configurations: NO
- The wire protocol: NO
- The rest endpoints: NO
- The admin cli options: NO
- Anything that affects deployment: NO
### Documentation
- Does this pull request introduce a new feature? NO
--
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] michaeljmarshall commented on a change in pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644#discussion_r635650389
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -976,7 +980,11 @@ public void deleteCursorFailed(ManagedLedgerException exception, Object ctx) {
}
void removeSubscription(String subscriptionName) {
- subscriptions.remove(subscriptionName);
+ PersistentSubscription sub = subscriptions.remove(subscriptionName);
+ // preserve accumulative stats form removed subscription
+ SubscriptionStats stats = sub.getStats(false, false);
Review comment:
Do you think it'd be worth adding specific getters for the two counters we want here? I am specifically concerned about the overhead of building out the whole `SubscriptionStats` object many times. I'm not sure _how_ expensive the `getStats` method really is, though.
--
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] dlg99 commented on a change in pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644#discussion_r635651182
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -976,7 +980,11 @@ public void deleteCursorFailed(ManagedLedgerException exception, Object ctx) {
}
void removeSubscription(String subscriptionName) {
- subscriptions.remove(subscriptionName);
+ PersistentSubscription sub = subscriptions.remove(subscriptionName);
+ // preserve accumulative stats form removed subscription
+ SubscriptionStats stats = sub.getStats(false, false);
Review comment:
`removeSubscription` does not happen that frequently.
--
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] michaeljmarshall commented on a change in pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644#discussion_r635654211
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -976,7 +980,11 @@ public void deleteCursorFailed(ManagedLedgerException exception, Object ctx) {
}
void removeSubscription(String subscriptionName) {
- subscriptions.remove(subscriptionName);
+ PersistentSubscription sub = subscriptions.remove(subscriptionName);
+ // preserve accumulative stats form removed subscription
+ SubscriptionStats stats = sub.getStats(false, false);
Review comment:
Oh, right. I had been thinking of consumers, not subscriptions.
--
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] eolivelli commented on pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644#issuecomment-844774865
@codelipenghui 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] codelipenghui merged pull request #10644: [Issue-10611] consumer related topic stats only available while consumer or reader are connected
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10644:
URL: https://github.com/apache/pulsar/pull/10644
--
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