fix: remove erroneous break in Stats() pubCounts loop#1526
fix: remove erroneous break in Stats() pubCounts loop#1526goingforstudying-ctrl wants to merge 2 commits into
Conversation
The break statement on line 325 caused the Stats() method to only record one topic per client when multiple topics were published to. This meant that when a client published to multiple topics, only the first topic's publish count would appear in statistics. Removing the break allows all topics to be collected correctly. This is a clear bug with user impact - anyone monitoring multi-topic client publish counts would see incomplete data.
|
Hi @mreiferson @ploxiln — this is a clear bugfix for multi-topic publish stats. Any chance someone could take a quick look? Happy to rebase if needed. |
|
CI is green, no conflicts. One-line bugfix removing an erroneous break in the Stats() loop. Ready for review. |
|
I see, this matters when (would be nice to have a test for this bit of behavior :) |
|
Good point — I'll add a test case for the multi-topic (blank topic) path and push an update shortly. |
Adds a test verifying that Stats() returns pubCounts for all topics when called with a blank topic name, and only the matching topic when called with a specific topic name. Addresses review feedback from ploxiln.
|
@ploxiln Added in — covers the blank-topic (all topics) and specific-topic filtering paths. Pushed. Let me know if you'd like anything else adjusted. |
|
@ploxiln Added TestClientStatsPubCounts in stats_test.go — it sets up an nsqd instance, publishes to two topics over the same producer connection, and asserts that both topics show up in the producer client stats with the correct counts. Confirmed it fails with the break in place and passes without it. |
|
I have awkwardly just realized that there are two older PRs with the same fix:
... what might justify this one is addressing the comment in #1497 :) |
|
@ploxiln thanks for catching that — I wasn't aware of #1525 and #1497. You're right that the test addition is the main differentiator here. The break causes only the first topic's pub count to be recorded when querying all topics, which is a real bug for anyone running multi-topic workloads. Happy to close this in favor of one of the older PRs if a maintainer prefers, but since the test is already written and pushed, I'll leave it open in case it's useful. Let me know if you'd like me to rebase or adjust anything. |
2 participants
Closing this PR as it's no longer needed.