KAFKA-19711: Add commit-rate metric to metered state stores Add commit metric#21853
Conversation
| final KafkaMetric metric = metric("flush-rate"); | ||
| assertTrue((Double) metric.metricValue() > 0); |
There was a problem hiding this comment.
The KIP mentions that while the flush metrics are only deprecated, they will no longer record any data under normal use, as Kafka Streams will no longer call StateStore#flush(), but this test seems to assert on a non-zero value. Is not recording flush metrics a next step?
There was a problem hiding this comment.
Good catch, no my intent was to do it in one step, that detail of the KIP slipped my mind. I'll update it.
…until next major release
|
@clolov I've updated the PR to only record for the |
aliehsaeedii
left a comment
aliehsaeedii
left a comment
There was a problem hiding this comment.
Thanks @bbejeck. I read the PR description and found it a bit confusing, which is why I left my earlier comment — and it looks like you’ve since updated the code based on another comment. It might be worth updating the PR description as well so they’re consistent.
Also, I couldn’t find the “varargs overload of StreamsMetricsImpl.maybeMeasureLatency” mentioned in the description; perhaps you ended up not needing it since you’re not measuring multiple metrics anymore.
| // flushSensor is deprecated per KIP-1035 and will be removed in the next major release. | ||
| // Recording a dummy value to avoid a SpotBugs URF_UNREAD_FIELD warning. | ||
| flushSensor = StateStoreMetrics.flushSensor(taskId.toString(), metricsScope, name(), streamsMetrics); | ||
| flushSensor.record(0.0); |
There was a problem hiding this comment.
The PR description states that “both sets of metrics are recorded during the deprecation period for backward compatibility”, but that’s not what the current code actually does.
I understand that the flush method has been removed, but when we talk about backward compatibility, my expectation would be that, until the next major release, both flushSensor and commitSensor should report/represent the same underlying behavior, and only in the next major release would we remove the deprecated flushSensor. That’s just my interpretation/expectation, though.
There was a problem hiding this comment.
Ok, now I know you did not intend to keep the flushSensore behaving the same as before. Just updating PR description is needed.
There was a problem hiding this comment.
@aliehsaeedii - I've updated the PR description to reflect the changes made for comments
…sor registration per deprecation plan
|
@aliehsaeedii I've updated the PR description and removed the |
clolov
left a comment
clolov
left a comment
There was a problem hiding this comment.
Looks great to me, thanks for the changes 😊
|
Merged #21853 into trunk |
…t metric (#21853) Add `commit-rate`, `commit-latency-avg`, and `commit-latency-max` metrics to replace the `flush-`* metrics which are now deprecated. The flush-* metrics will be removed in the next major release. Reviewers: Eduwer Camacaro <eduwerc@gmail.com>, Chriso Lolov <clolov@apache.org>, Alieh Saeedii <asaeedi@confluent.io>
|
cherry-picked to 4.3 |
5 participants
Add
commit-rate,commit-latency-avg, andcommit-latency-maxmetrics to replace the
flush-* metrics which are now deprecated. Theflush-* metrics will be removed in the next major release.
Reviewers: Eduwer Camacaro eduwerc@gmail.com, Chriso Lolov
clolov@apache.org, Alieh Saeedii asaeedi@confluent.io