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
[FLINK-24409][connectors] Fix metrics errors with topics names with periods #17401
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit fd01adb (Fri Oct 01 06:35:44 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
@PatrickRen could you PTAL? I think we should have a test case for record lag metric 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.
@PatrickRen could you please give pointers to @jherico on how to approach the test? |
Sorry for my late response! @jherico Here's my implementation of the test case that might be helpful. Feel free to cherry-pick it or implement your own. Another choice is to use parameterized test in JUnit 5 for testing |
@jherico Do you think you could add the tests or do you need some more help? |
I can most likely add some tests, I've just been very busy lately and it
hasn't been a priority. Will try to see if I can manage in the next week
or so.
…On Wed, Nov 10, 2021 at 3:58 AM MartijnVisser ***@***.***> wrote:
@jherico <https://github.com/jherico> Do you think you could add the
tests or do you need some more help?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17401 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALWNSSFRJKQ5RYJ6WJPFUDULJM5PANCNFSM5FD6JTWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
That would be great. There are multiple contributors who would like to help out (it's a bug that we would like to resolve asap) so do let know in case you need any help. |
Does it make sense to re-assign the task to someone else? |
ok, it's a pretty small change I know, but I'm kind of irked that #17773 appears to include my exact changes but without my actual commit. |
@jherico sorry that was my fault :( I did not think about when copying your code instead of cherry-picking the commit. |
What is the purpose of the change
topic
tag, but the metric tags execute a transform on the topic name that replaces any periods with underscores, so if you try to find the records-lag metric in via the changed function, it logs a WARN level log with an exception stack trace.Brief change log
getRecordsLagMetric
functionVerifying this change
This change is a trivial rework / code cleanup without any test coverage.
Unfortunately while some of the tests in
KafkaSourceReaderTest
cover this code and changing the topic name in that class to"Kafka.Source.Reader.Test"
even causes the issue, it doesn't actually cause the tests to fail. Further, theKafkaSourceReaderMetricsTest
doesn't have any coverage of thegetRecordsLagMetric
function at all, which is disappointing. I tried to add some coverage but I wasn't successful.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation