-
15. Re: Race condition found by failure in GroupingFailoverRepli
"clebert.suconic@jboss.com" wrote:
I have a fix already, but I couldn't commit it today. If you guys are interested on it, I have attached a patch to the JIRA.
I have applied your patch but it did not prevent the issue. The packet you send to the backup server is send *after* messages have been added to the queue. It does not prevent out of order delivery.
However, based on your patch, I have modified the code in PostOffice.processRoute():private void processRoute(final ServerMessage message, final RoutingContext context) throws Exception { if (message.isDurable() && queue.isDurable()) { // lots of storageManager operations } else { // make sure to preserver ordering even for non-persistent messages storageManager.sync(); } ... if (storageManager.isReplicated()) { storageManager.afterReplicated(new Runnable() { public void run() { addReferences(refs); } }); } else { addReferences(refs); } } }
StorageManager.sync() method (the name is not good) will send a packet to the backup server to ensure
that even non persistent messages will be added to the queue after replication.
The method delegates to Journal.sync() which is a no-op for the JournalImpl implementation. For ReplicatedJournal, it send a packet using the ReplicationManager to ensure order.
With this modification, I no longer see the failure on the test.
wdyt? -
16. Re: Race condition found by failure in GroupingFailoverRepli
I was thinking ... it should be very easy to guarantee order without the roundtrip to the backup server.
We could just add a special token on ReplicationManager::pendingTokens. When the responses are back, it always poll from the tokens, and if it is the "special" token, we execute it and call the next one.
We could also do some checks while adding the tokens. We could just process the token right away without going to the replicated server, this way we wouldn't need to wait replication when we don't need to. -
17. Re: Race condition found by failure in GroupingFailoverRepli
"clebert.suconic@jboss.com" wrote:
I was thinking ... it should be very easy to guarantee order without the roundtrip to the backup server.
We could just add a special token on ReplicationManager::pendingTokens. When the responses are back, it always poll from the tokens, and if it is the "special" token, we execute it and call the next one.
We could also do some checks while adding the tokens. We could just process the token right away without going to the replicated server, this way we wouldn't need to wait replication when we don't need to.
This won't work since you'll always need a real replication some time after the special token in order to cause it to execute. But you can't guarantee a real replication will occur or when. -
18. Re: Race condition found by failure in GroupingFailoverRepli
There is, however, an optimisation that can be done here.
If replicating a dummy packet, and there are no real replications in the queue, waiting for responses, then the dummy response can be released immediately, and order won't be broken. -
19. Re: Race condition found by failure in GroupingFailoverRepli
"timfox" wrote:
"clebert.suconic@jboss.com" wrote:
I was thinking ... it should be very easy to guarantee order without the roundtrip to the backup server.
We could just add a special token on ReplicationManager::pendingTokens. When the responses are back, it always poll from the tokens, and if it is the "special" token, we execute it and call the next one.
We could also do some checks while adding the tokens. We could just process the token right away without going to the replicated server, this way we wouldn't need to wait replication when we don't need to.
This won't work since you'll always need a real replication some time after the special token in order to cause it to execute. But you can't guarantee a real replication will occur or when.
I wouldn't add the token if there are no replication. I would release it immediately.
I would do:
if there are replication.. add the special token..
if not: release the token imediately since there is no pending replication, and the order would be guaranteed.
When receiving the responses.. if it finds the special token... we release it and go to the next token.
That's in summary what I said. -
20. Re: Race condition found by failure in GroupingFailoverRepli
Can someone make sure there is a test which sends durable and non durable messages in the same session, replicated, and asserts the order of consumption is correct?
-
21. Re: Race condition found by failure in GroupingFailoverRepli
Also was this deliberate, in a recent commit (rev 8280): ?
[code}
message.setDestination(queueName);
+// message.setDurable(true); -
22. Re: Race condition found by failure in GroupingFailoverRepli
What is the point of this sync() method?
All it does it send a packet to the backup, I don't get it.
Like I mentioned in my previous posts what needs to be done is references added on the callback *irrespective* of whether they are durable or not.
Also, like I also mentioned in a post, an optimisation can be done to execute immediately if nothing is on the queue. -
23. Re: Race condition found by failure in GroupingFailoverRepli
References are always added thorugh the callback when replicated. That was not the original problem.
Nothing was going through the backup. So, as soon as the context was closed it would complete all the operations since nothing was persistent and nothing was sent to the backup.
Notice that the callbacks won't be executed until context.close is called (and all the packets are back from the replicated server).
If nothing is replicated. As soon as you hit context.close, the callbacks are executed.
My original fix was to guarantee a packet sent to the replicated server if nothing was persistent. (not on every operation though. Only when nothing was persistent, and only once per replication context). It would be possible actually to avoid the round trip on the server altogether. (ATM I have created a JIRA for 2.1. We could place it back to 2.0, and it should be an easy task - https://jira.jboss.org/jira/browse/HORNETQ-205).
Jeff then created a method sync (I also reviewed it) that would just send the "dummy packet" on the replicated server to make sure there is an operation to be replicated on the context. This will actually behave exactly the same as doing it through the close before. (Notice also that the test is currently failing on hudson. As i said it would behave the same, and there's probably something else going on). -
24. Re: Race condition found by failure in GroupingFailoverRepli
"timfox" wrote:
Also was this deliberate, in a recent commit (rev 8280): ?message.setDestination(queueName); +// message.setDurable(true);
I added this commented code while working on the race condition.
As I said in a previous post, conversely to management notifications which are durable, messages sent by PostOffice.sendInfoQueueToQueue are not.
I am not sure if they should be durable too but it'd make sense to factorize code to create them as the regular management notifications. -
25. Re: Race condition found by failure in GroupingFailoverRepli
"jmesnil" wrote:
"timfox" wrote:
Also was this deliberate, in a recent commit (rev 8280): ?message.setDestination(queueName); +// message.setDurable(true);
I added this commented code while working on the race condition.
As I said in a previous post, conversely to management notifications which are durable, messages sent by PostOffice.sendInfoQueueToQueue are not.
I am not sure if they should be durable too but it'd make sense to factorize code to create them as the regular management notifications.
It's best to fix the bug first, then think about changing durable later. Otherwise making them both durable or non durable might mask the bug.
(Also don't commit commented out code - if it's not needed, delete it) -
26. Re: Race condition found by failure in GroupingFailoverRepli
"clebert.suconic@jboss.com" wrote:
Jeff then created a method sync (I also reviewed it) that would just send the "dummy packet" on the replicated server to make sure there is an operation to be replicated on the context. This will actually behave exactly the same as doing it through the close before. (Notice also that the test is currently failing on hudson. As i said it would behave the same, and there's probably something else going on).
This sync() method seems to be called inside the inner loop in PostOfficeImpl:processRoute and one sync packet will be sent for each queue routed to.
This seems an unnecessary number of sync packets - surely we only need to send one per route, if one hasn't already been sent?
We also need tests to make sure this works with both non transactional and transactional cases - have such tests been written yet? -
27. Re: Race condition found by failure in GroupingFailoverRepli
There was in fact an issue with ordering on transactional.
At TransactionImpl::commit, The replication manager won't get informed about the callback, and the Transaction would execute the callback directly.
The test that Jeff wrote didn't get any failure (even after my changes to make sure nothing would be persistent on the context).
I'm making a few modification making the Context more generic to the Journal so I'm fixing that on a branch (but it will be committed shortly).
So, @Jeff and @Andy: I"m not sure if the Grouping thing is playing with transactions. If it is there's a possibility for a race here. -
28. Re: Race condition found by failure in GroupingFailoverRepli
@Jeff and @Andy: I just looked at GroupingFailoverTestBase. It doesn't use transactions, so I think you're safe here.
-
29. Re: Race condition found by failure in GroupingFailoverRepli
"clebert.suconic@jboss.com" wrote:
@Jeff and @Andy: I just looked at GroupingFailoverTestBase. It doesn't use transactions, so I think you're safe here.
This is a good point though - are there clustered grouping tests which do use transactions? If not, there should be.