You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jbertram <gi...@git.apache.org> on 2017/08/16 19:43:26 UTC
[GitHub] activemq-artemis pull request #1466: ARTEMIS-1353 ensure replication packet ...
GitHub user jbertram opened a pull request:
https://github.com/apache/activemq-artemis/pull/1466
ARTEMIS-1353 ensure replication packet order
Incorrect ordering of replication packets may happen because of
useExecutor parameter in the sendReplicatePacket method.
ReplicationStartSyncMessage packets are sent as first, but they are sent
with useExecutor=true. Although ReplicationSyncFileMessage packets are
sent after ReplicationStartSyncMessage packets, they are sent with
useExecutor=false. So sending of ReplicationStartSyncMessage packets is
scheduled to executor and there is no guarantee when the task will be
executed, whereas ReplicationStartSyncMessage packets are sent
immediately.
The solution is to wait for an ack for ReplicationStartSyncMessages.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1353
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1466.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1466
----
commit a9f3dcfb10314763d5feeff6d56de9694cc0ede1
Author: Justin Bertram <jb...@apache.org>
Date: 2017-08-16T17:08:31Z
ARTEMIS-1353 ensure replication packet order
Incorrect ordering of replication packets may happen because of
useExecutor parameter in the sendReplicatePacket method.
ReplicationStartSyncMessage packets are sent as first, but they are sent
with useExecutor=true. Although ReplicationSyncFileMessage packets are
sent after ReplicationStartSyncMessage packets, they are sent with
useExecutor=false. So sending of ReplicationStartSyncMessage packets is
scheduled to executor and there is no guarantee when the task will be
executed, whereas ReplicationStartSyncMessage packets are sent
immediately.
The solution is to wait for an ack for ReplicationStartSyncMessages.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1466: ARTEMIS-1353 ensure replication packet ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1466
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
The wait is in ReplicationManager#sendStartSyncMessage. It's essentially the same kind of thing that's done in ReplicationManager#sendSynchronizationDone.
BTW, I ran the full test-suite. No regressions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
Please wait me to merge this? I want to review this carefully tomorrow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
Clebert worked up a simpler solution so I'm closing this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
@clebertsuconic Rather than having all this code to synchronise the sending of large files via the same thread and executor. Why not just use an executor with a blocking queue for all?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
Where did you add the wait ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
@clebertsuconic It appears the original issue was introduced when you changed the ReplicationManager to skip the executor and send in the same thread. I am trying to understand why this was required. From a quick read of the code, I assume that when sending a large file there's potential for the whole file to get scheduled on the ThreadPoolExecutor Queue, with potential to cause an OOM? Is this assumption correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
I'm not sure how you would do that.
I would send 10 and some logic that would resume it. Like new runnable () {resumeCall(); }
This seemed a simple solution when I fixed it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
I want to do some review on the extra wait. Wait for me to merge this ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1466: ARTEMIS-1353 ensure replication packet order
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1466
if it was using the executor it would OME for sure.. I believe that's why I did it back then...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---