-
Notifications
You must be signed in to change notification settings - Fork 64
Use the same context to guarantee message delivery order #52
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
Conversation
@vietj, can you review the changes? |
we need to come up with a test for this |
any test for this ? |
@vietj, sure, I will work on it |
…e_order # Conflicts: # src/test/java/io/vertx/rabbitmq/RabbitMQServiceTest.java
@vietj , just done |
what is the actual thread if we remove run on context ? |
the solution appears faulty to me because we should be on the vertx thread and the correct solution would rather be to capture somewhere the context and use context#runOnContext rather than vertx.runOnContext |
So |
@vietj, can we catch a context in the ConsumerHandler constructor by calling vertx#getOrCreateContext and then use context#runOnContext for handling? |
})); | ||
|
||
awaitLatch(latch); | ||
testComplete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testComplete should not be called here, it is only necessary when called asynchronously and if you call await()
in the junit thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
assertNotNull(json); | ||
String body = json.getString("body"); | ||
assertNotNull(body); | ||
log.info("received: " + body + " expected: " + expectedMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
channel.basicPublish("", queueName, properties, msg.getBytes("UTF-8")); | ||
|
||
vertx.eventBus().consumer(address, msg -> { | ||
String expectedMessage = receiveOrderQueue.poll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather collect all messages in a synchronized list, remove the latch and use assertWaitUntil(() -> list.size() == 1000) in the main thread, it will be more clear and easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and check the message order in the junit thread after assertWaitUntil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
AMQP.BasicProperties properties = new AMQP.BasicProperties.Builder().contentType("text/plain").contentEncoding("UTF-8").build(); | ||
|
||
// send messages | ||
for (String msg : sendingOrder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should likely publish messages after having etup the event bus consumer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very important to have unconsumed messages in rabbitmq before running a consumer, that is how the problem were discovered. The order might looks like fine when publishing and instantly receiving because of delay between arrived messages.
That is what @tibor-kocsis saying about issue reproducing:
If you run the Sender, then a few minutes later the Receiver, the output will be the following
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, are we sure then this is enough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is enough
looks ok now |
A solution for #51.