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

New feature: message with priority #13

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

StonesGitHub
Copy link

@StonesGitHub StonesGitHub commented Feb 12, 2018

I found that if the queue in rabbitmq is a queue with priority, it will report error:
x-max-priority is none, but the queue is not
To support sending message with priority, a few changes have been made to enable priority queue

@StonesGitHub StonesGitHub changed the title New feature New feature: message with priority Feb 13, 2018
Copy link
Owner

@crabhi crabhi left a comment

Choose a reason for hiding this comment

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

Thank you for this patch and I'm sorry for getting to you a bit late.

I don't have the priority queues set up in production so it's a bit harder for me to be confident about these changes.

Could you please add a test case? At least into the integration tests in the examples module. It works by spawning a RabbitMQ in a Docker container and then doing end-to-end calling of a task through this real Rabbit instance.

@Nullable final ExecutorService executor) {
@Nullable final ExecutorService executor,
@Nullable final boolean isPriQueue,
@Nullable final int maxPriority) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could maxPriority be an Optional<Integer>? That way you wouldn't need isPriQueue parameter.

* @return asynchronous result
* @throws IOException
*/
public AsyncResult<?> submitWithPri(String name, int priority, Object[] args) throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer if this and other methods kept the same name as the existing ones, just overloaded by parameters.

@StonesGitHub
Copy link
Author

Hi @crabhi , I have made some changes accordingly, as well as supplying a test case(I have tested it on my local machine).
Pls review the code. ^_^

@todd-cook
Copy link

Curious what the status of this is @crabhi

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