-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use TopicPartition.topic for metrics #235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh very nice catch...! I overlooked that these two topic references may refer different topics in case of retry and indeed it should refer to the currently consuming topic rather than the origin subscription topic.
@@ -59,7 +59,7 @@ protected AbstractSubPartitions(PartitionScope scope, Processors<?> processors) | |||
rateLimiter = new DynamicRateLimiter(rateProperty(scope)); | |||
Metrics metrics = Metrics.withTags( | |||
"subscription", scope.subscriptionId(), | |||
"topic", scope.topic(), | |||
"topic", scope.topicPartition().topic(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in addition to these changes I wanna make the following changes to ensure we won't repeat the same mistake in future. Are you happy to follow these up in this PR? Otherwise I'll cover that after merging this PR.
- Let's use variable to assign
scope.topicPartition()
return and use it instead to avoid unnecessary object allocation. - Rename
SubscriptionScope.topic
to beSubscriptionScope.originTopic
to distinguish it w/ PartitionScope.topicPartition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review. I see, I'll update them as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This has been released as 8.0.1. Thanks for your contribution :) https://github.com/line/decaton/releases/tag/v8.0.1 |
OK, thanks for your help. |
scope.topic()
after Add virtual threads support #224.scope.topicPartition().topic()
to use the retry topic name for metrics when handling the retry topic.