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

fix: detect the missing of rabbitmq queue and redeclare it. #556

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

JetDrag
Copy link
Contributor

@JetDrag JetDrag commented Jun 29, 2023

The declared rabbitmq queue will be missing sometime, such as deletion by mistake or rabbitmq server failure. So I add some logic to make sure the missing queue to be declared again, as the recent implementation won't auto re-declare the missing queue.

There are two situation to re-declare the missing queue:

  • Declare the queue every time when the consumer start.
  • Redeclare the queue when queue is found to be missing during enqueue.

@JetDrag
Copy link
Contributor Author

JetDrag commented Jul 4, 2023

Hi, @Bogdanp , do you have time to review this fix? Many thanks for your work!

@JetDrag
Copy link
Contributor Author

JetDrag commented Aug 28, 2023

Hi, @Bogdanp , do you have time to review this fix? I've update a newer version. We have run Dramatiq with this fix for a while at our production environment and it's necessary for long-running rabbitmq workers to include this fix to ensure reliability.

Thanks for your time.

@Bogdanp
Copy link
Owner

Bogdanp commented Aug 30, 2023

Hi @JetDrag, I'll try to review this PR sometime around the end of next week.

@JetDrag
Copy link
Contributor Author

JetDrag commented Aug 31, 2023

@Bogdanp roger, thanks

@JetDrag
Copy link
Contributor Author

JetDrag commented Sep 23, 2023

Hi, @Bogdanp , I've resolve the conversation you added before, please review again.

tests/test_rabbitmq.py Outdated Show resolved Hide resolved
@JetDrag
Copy link
Contributor Author

JetDrag commented Oct 22, 2023

Hi, @Bogdanp , I've resolve the conversation you added before, when will you have time to review the renewed fix?

We have run Dramatiq with this fix for a while at our production environment and it's necessary for long-running rabbitmq workers to include this fix to ensure reliability.

Many thanks!

@JetDrag
Copy link
Contributor Author

JetDrag commented Apr 20, 2024

Hi, @Bogdanp , I've resolve the failure of lint, make sure the test cases in workflow is passed in my forked project. Could you approve the workflow in this PR?

@Bogdanp
Copy link
Owner

Bogdanp commented Apr 21, 2024

I see both my comments were marked as resolved, but they don't seem to be addressed?

@JetDrag
Copy link
Contributor Author

JetDrag commented Apr 21, 2024

I see both my comments were marked as resolved, but they don't seem to be addressed?

Sorry, there's something missing during rebase. I've updated the code to make sure your comments are resolved. And I also add some enhancements to make sure the concurrent running tests will passed.

Please approve the workflow and review again. @Bogdanp

@Bogdanp
Copy link
Owner

Bogdanp commented Apr 23, 2024

The suggestion in https://github.com/Bogdanp/dramatiq/pull/556/files#r1334929506 still doesn't seem to be addressed. The idea is to avoid adding a strict argument, and instead make ensure take True, False or "strict".

@JetDrag
Copy link
Contributor Author

JetDrag commented Apr 24, 2024

Sorry for the omission, it seem there're some bugs in the online github CodeSpace. I've refined and pushed it again.

Thank you for your patience and work.

@Bogdanp Bogdanp force-pushed the fix/ensure_queue branch 2 times, most recently from 7391a36 to 6b2311d Compare April 28, 2024 06:17
@Bogdanp Bogdanp merged commit 4d0cc07 into Bogdanp:master Apr 29, 2024
11 checks passed
@JetDrag
Copy link
Contributor Author

JetDrag commented May 15, 2024

Awesome work! Thanks to your patient and meticulous!

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.

3 participants