lizhimins commented on PR #4512: URL: https://github.com/apache/rocketmq-dashboard/pull/4512#issuecomment-5761155057
Thanks for the detailed PR and for linking the Tencent documentation on `ConsumerLag` being nullable — that part is accurate, and the change is well built. To summarise what it does, so we are talking about the same thing: a null `SubscriptionData.ConsumerLag` on the Tencent path stops mapping to `0` and instead becomes `-1` through the new `toConsumerLag` helper (both call sites, group progress and topic consumer); the topic-consumer path additionally derives `metricsAvailable` from that value; and `CloudRocketMqBusinessMetricsCollector` drops its `Math.max(0, ...)` clamp so an unknown row turns `consumer.lag.total` and `consumer.lag.max_queue` into `UNAVAILABLE` samples, with `topic.backlog.total` distinguished per topic and `clusterId` attached to the unavailable samples. Reusing the existing `ConsumerLagResolver.UNKNOWN` / `metricsAvailable` contract rather than inventing a new one was the right instinct, and the three new tests are mutation-sensitive. **We are not taking this one.** On our reading, when the Tencent monitoring API does not report a `ConsumerLag` for a subscription, `0` is the value we want on this surface. This is the same position we gave on #4386 and recorded in #4361: the backlog numbers an operator reads during an incident stay plain numerics, and we do not want a second, overlapping representation of "no data" that has to be kept consistent with the first by hand. To be clear about the boundary of that decision: the `ConsumerLagResolver.UNKNOWN` / `metricsAvailable` contract on the Apache queue and topic lag paths stays exactly where it is — we are simply not extending it to the cloud providers. Two further things the review noted, which are independent of the decision above: 1. The change was not symmetric across the cloud providers. `AliyunConverters.toQueueProgressRows` still maps a null `readyCount` to `0L`, and `CloudRocketMqBusinessMetricsCollector` serves both Aliyun and Tencent instances — so the new unknown branch would never fire for an Aliyun instance. `AliyunConverters.toTopicConsumerVO` does not set `diffTotal` at all, which defaults to `0` with `metricsAvailable` true. 2. The sentinel was written as the literal `-1L` rather than `ConsumerLagResolver.UNKNOWN`, and the collector's test was `< 0` rather than `== UNKNOWN`, which does not match the predicate #4517 introduced in `ApacheRocketMqBusinessMetricsCollector` (`< 0` also swallows a genuinely negative lag reported by the cloud). That said, the decision rests on treating "the API reported no value" as "no backlog", and I may be wrong about that. If you have a concrete case where Tencent returns a null `ConsumerLag` while backlog genuinely exists — ideally a captured API response body plus what the console showed at the same moment — please reopen with that reproduction and we will look again. A real capture is exactly the evidence that would change the answer. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
