Skip to content

Conversation

@ikss
Copy link

@ikss ikss commented Nov 6, 2025

Proposed Changes

Add support for if-empty and if-unused flag for quorum queues deletion

These flags are supported for classic queues, but for quorum queues they are not supported.
Implementation looks pretty straightforward and tests in ct-quorum_queue suite are green, but I'm not proficient in elixir and this project. So please advice how it can be done better.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
This is simply a reminder of what we are going to look for before merging your code.

  • Mandatory: I (or my employer/client) have have signed the CA (see https://github.com/rabbitmq/cla)
  • I have read the CONTRIBUTING.md document
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

One question still bothers me that now state function will return ok even on error/timeout which may cause deletion of queue in which state we're not sure. Please suggest how to tackle it if it should be

@ikss ikss changed the title Feature/qq delete is empty flag Feature/support if-empty and if-unused in quorum queues Nov 6, 2025
@ikss ikss changed the title Feature/support if-empty and if-unused in quorum queues Feature/support if-empty and if-unused flags in quorum queues Nov 6, 2025
@michaelklishin
Copy link
Collaborator

@ikss this is not Elixir that you are modifying, this is Erlang. Did you use an LLM to generate these changes? If so, always disclose that because some companies may or may not be legally allowed to accept such contributions.

In any case, we need all first time contributors to digitally request and sign a CLA, which it less than two pages long.

@michaelklishin
Copy link
Collaborator

michaelklishin commented Nov 6, 2025

@ikss FYI, merging main (as opposed to rebasing) into a topic branch means we won't be able to backport this change ae08eec.

So after signing the CLA, please submit a topic branch without any branches merged into it.

@michaelklishin
Copy link
Collaborator

michaelklishin commented Nov 6, 2025

Finally, as another maintainer pointed out to me:

%% TODO Quorum queue needs to support consumer tracking for IfUnused

was removed in this PR without any related changes that I see.

Are you sure that stat/1 will return up-to-date enough consumer count state?

@ikss
Copy link
Author

ikss commented Nov 6, 2025

Hello @michaelklishin , thank you for your comments

this is not Elixir that you are modifying, this is Erlang. Did you use an LLM to generate these changes?
always disclose that

Yes, I iterate on this PR with Claude Code, since my primary toolstack doesn't include Erlang and it's ecosystem. I understand how such LLM-based PRs could bloat opensource maintainers and I'm sorry for not putting it in description

In any case, we need all first time contributors to digitally request and sign a CLA

I did it first thing before creating a PR, so I checked that checkbox in the PR

%% TODO Quorum queue needs to support consumer tracking for IfUnused

I checked in the git history when this comment was added (in the very first initial commit in 2018) and what changes were done after that. And to my best understanding consumers tracking was added in later PRs and last time changed in Quorum queues v4 implementation where consumer priorities were added for SAC, so I assumed that consumer state should be up to date for supporting those features

I wanted to add support for if-empty flag only at first, because in our usecase (creating and deleting a lot of quorum queues dynamically) we would benefit from it. So if consumer state tracking need some further adjustments is it OK to create a PR only for if-empty?

@michaelklishin
Copy link
Collaborator

Sorry, we won't accept this PR.

Not only there are questions to the code produced here and the branch is tainted by a main merge, it's generally very difficult to take seriously a PR whose author cannot even name the programming language they are modifying.

@rabbitmq rabbitmq locked and limited conversation to collaborators Nov 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants