birdayz / kaf

Modern CLI for Apache Kafka, written in Go.
Apache License 2.0
2.24k stars 143 forks source link

Improve lag calculation and error handling in lagCmd #341

Closed jackcipher closed 4 months ago

jackcipher commented 5 months ago

This PR enhances the lagCmd function to improve its reliability and error reporting. The main changes are:

  1. Added a check to detect and report cases where the consumer offset is greater than the high watermark. This helps identify potential inconsistencies in the data.

  2. Improved error messages for better clarity when troubleshooting issues.

  3. Enhanced resource management by ensuring the admin client is properly closed using defer.

  4. Only report lag values greater than zero, reducing noise in the output.

  5. Minor code improvements for readability (e.g., renaming variables).

These changes should make the lag calculation more robust and informative, especially in edge cases or when dealing with topic compaction or message retention scenarios.

Testing:

Please review and let me know if any further changes are needed.

birdayz commented 4 months ago

Only report lag values greater than zero, reducing noise in the output.

i would argue, that this is very counter intuitive. when i just tested it locally, i was confused because my group was not listed at all. i would strongly prefer zero lag being shown.

also, zero lag is not such a common case..in local testing or low-traffic cases, yes. however, if you do not have completely trivial scale, lag will rarely be zero (as commits happen after some time / a processed batch, not immediately).

So, i'd rather not have special treatment for zero.

i'm ok with the rest of the changes.

thanks!

jackcipher commented 4 months ago

Thank you for the feedback. I agree that showing zero lag is important especially during local testing or in low-traffic scenarios.

so I have made the following updates:

birdayz commented 4 months ago

thank you!