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

Allow usage of if-empty, if-unused parameters for queue deletion #759

Merged

Conversation

jdfresser
Copy link
Contributor

@jdfresser jdfresser commented Feb 9, 2024

This closes ##747

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
It probably misses some tests in https://github.com/rabbitmq/messaging-topology-operator/pull/759/files#diff-5619a2073d5cfff0b764ead1f1323f7554a40d748c906bcaa1322f1dd8a649dfR103

Summary Of Changes

  • Support 'failsafe' strategies to prevent the operator to delete queues with consumers/messages
  • Modifies the API with two new 'mutables' fields in Queue CRD: Spec/DeleteIfEmpty & Spec/DeleteIfUnused
  • For Quorum queues, check the rabbitMQ HTTP API to know if there are consumers/messages
  • For other queues, use the built-in parameters

@vmwclabot
Copy link

@jdfresser, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@jdfresser, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@Zerpet
Copy link
Contributor

Zerpet commented Feb 14, 2024

Thank you for this contribution! I had a quick look and it's looking good overall. I want to check something with the team, because I'm unsure why Quorum Queues do not support if-empty and if-unused. Perhaps we could add that to the core, and that would simplify a lot this PR.

internal/queue_delete_options.go Outdated Show resolved Hide resolved
controllers/queue_controller.go Outdated Show resolved Hide resolved
controllers/queue_controller.go Outdated Show resolved Hide resolved
controllers/queue_controller.go Outdated Show resolved Hide resolved
system_tests/queue_system_test.go Outdated Show resolved Hide resolved
@jdfresser
Copy link
Contributor Author

@Zerpet, Thank you for your review, I applied the suggested changes.

I want to check something with the team, because I'm unsure why Quorum Queues do not support if-empty and if-unused. Perhaps we could add that to the core, and that would simplify a lot this PR.

I'm also curious about the reason. Yes, better to avoid work-aroundsif we can!

Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I'm going to do some manual QA before merging.

@Zerpet
Copy link
Contributor

Zerpet commented Feb 20, 2024

@Zerpet, Thank you for your review, I applied the suggested changes.

I want to check something with the team, because I'm unsure why Quorum Queues do not support if-empty and if-unused. Perhaps we could add that to the core, and that would simplify a lot this PR.

I'm also curious about the reason. Yes, better to avoid work-aroundsif we can!

@jdfresser the main reason is that we can't deliver on either promise without some racy conditions. In QQ, a publisher can publish to any member (usually 3) and a consumer can consume from any replica. This fact makes it harder to deliver on "if empty" or "if unused" without some limitations. It' not the same case for classic queues, CQ can deliver both promises.

FYI I opened this issue to support "if empty" and "if unused" in QQs, but it won't make it until rabbit 4.0: rabbitmq/rabbitmq-server#10543

@Zerpet
Copy link
Contributor

Zerpet commented Feb 20, 2024

CI failed because there are unused imports in internal/queue_delete_options.go. It fails to build and go vet. Can you fix those?

@jdfresser
Copy link
Contributor Author

Sorry, stup*d me forgot to remove unused imports and to check local-tests results.

Local-tests are running well now.

@MirahImage MirahImage merged commit 645408b into rabbitmq:main Feb 28, 2024
5 checks passed
@Zerpet Zerpet added this to the v1.14 milestone Feb 29, 2024
@vmwclabot
Copy link

@jdfresser, VMware has approved your signed contributor license agreement.

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

Successfully merging this pull request may close these issues.

4 participants