Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly close internal reply queues when collecting queue stats #8349

Closed
wants to merge 3 commits into from

Conversation

florimondmanca
Copy link
Contributor

What does this PR do?

Close internal reply queue when collecting queue stats.

Motivation

A customer reported that "the Agent writes to a dead-letter queue associated to PYMQPCF.*, but we monitor on all DLQ so this causes alerts in production", and they would like the Agent to not submit anything to these DLQ. These writes occur in parallel to them seeing a WARNING in the Agent logs due to a failure to fetch the queue stats.

My understanding is that PYMQPCF.* is the prefix of internal queues (known as "reply queues") used by pymqi when executing a command. Then here's my best theory of what happens with the DLQ:

  • Under normal conditions, the command succeeds and pymqi pops messages from those queues to form the final response, leaving no undelivered messages and nothing in the DLQ.
  • But if an exception occurs, some messages may be left in the reply queue, and because we don't dispose it explicitly they will be left hanging there, marked as "undelivered", and moved to a DLQ. By calling pcf.disconnect(), the reply queue gets properly closed, so no messages will be marked as "undelivered" since the queue is disposed.

I will create a beta off this branch to see if this theory holds and if it resolves the customer's issue with DLQ items. If it does, I will apply this finally block to everywhere we use PCFExecute.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants