terascope / kafka-assets

teraslice asset for kafka operations
MIT License
1 stars 1 forks source link

Fix Kafka producer queue full bug #861

Open sotojn opened 1 week ago

sotojn commented 1 week ago

This PR makes the following changes:

Ref to issue #755

sotojn commented 1 week ago

I will be adding debug logging for the producer flush() call in the morning. Moving back to draft until then.

godber commented 13 hours ago

On Friday, when reviewing this I remembered there are TWO reasons I wanted you to start test jobs creating these conditions. FIRST to have evidence that the conditions are triggering as needed. SECOND so you can read the logs in the context that they are being printed and assess whether the wording is as effective ... in context of other logs ... as they can be.

Your log messaging is improved for sure, but take a look at worker logs on your test job, and think about what can be improved. There's one word you can add to the message that would improve it immensely. This word is not obvious when you're implementing it because the context of your change is super obvious, but in the mess of other log messages, from the perspective of the user/operator it is less obvious. Technically the word is in the output, but not in the log message itself.

Respond as a comment here, then we'll wrap up this PR.