[GitHub] activemq-artemis pull request #1428: ARTEMIS-1308: Delegate acknowlegde to C...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1428: ARTEMIS-1308: Delegate acknowlegde to C...

franz1981
GitHub user michaelandrepearce opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1428

    ARTEMIS-1308: Delegate acknowlegde to ClientMessage

    ActiveMQMessage acknowledge should delegate to ClientMessage acknowledge

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1308

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1428.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 #1428
   
----
commit a1887fb8271e1e660d16683f4dc6a62309feeaf4
Author: Michael Andre Pearce <[hidden email]>
Date:   2017-07-28T13:27:29Z

    ARTEMIS-1308: Delegate acknowlegde to ClientMessage
   
    ActiveMQMessage acknowledge should delegate to ClientMessage acknowledge

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    How will the ack get to the broker if you don't commit the session?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    This is def a WIP for a possible solution, but made some progress in IRC chat. don't think we are there yet entirely.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    This is def improvement over the first patch I saw.  


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    would be too much to ask you to send a PR on the 1.x branch?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    This is causing a few failures on the testsuite:
   
   
    Test Name | Duration | Age
    -- | -- | --
    org.apache.activemq.artemis.jms.tests.MessageConsumerTest.testRedeliveredDifferentSessions | 54 ms | 1
    org.apache.activemq.artemis.jms.tests.MessageConsumerTest.testClientAcknowledgmentOnClosedConsumer | 1 sec | 1
    org.apache.activemq.artemis.jms.tests.MessageConsumerTest.testRedel7 | 31 ms | 1
    org.apache.activemq.artemis.jms.tests.message.BytesMessageTest.testRedelivery | 52 ms | 1
    org.apache.activemq.artemis.jms.tests.message.MapMessageTest.testRedelivery | 38 ms | 1
    org.apache.activemq.artemis.jms.tests.message.ObjectMessageTest.testRedelivery | 33 ms | 1
    org.apache.activemq.artemis.jms.tests.message.StreamMessageTest.testRedelivery | 26 ms | 1
    org.apache.activemq.artemis.jms.tests.message.TextMessageTest.testRedelivery | 23 ms | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignBytesMessageTest.testRedelivery | 30 ms | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignMapMessageTest.testRedelivery | 0.18 sec | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignMessageTest.testRedelivery | 37 ms | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignObjectMessageTest.testRedelivery | 30 ms | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignStreamMessageTest.testRedelivery | 29 ms | 1
    org.apache.activemq.artemis.jms.tests.message.foreign.ForeignTextMessageTest.testRedelivery | 34 ms | 1
   



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    Yeah seems that we need to somehow ensure when redelivery or consumer.close is called to ensure any waiting batched packets are send and confirmed. It seems some acks are still in batches waiting to be sent. (i tried also doing the other option you suggested Clebert with an async ack, instead of using  auto ackcommit on the session it has same problem) ill look to see what options there are


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @clebertsuconic @franz1981 i believe i have this PR working except i am getting one test failure, see below.
   
    https://builds.apache.org/job/ActiveMQ-Artemis-PR-Build/3570/testReport/junit/org.apache.activemq.artemis.tests.unit.core.journal.impl/TimedBufferTest/testTimeOnTimedBuffer/
   
    Is this related at all to:
    https://github.com/apache/activemq-artemis/pull/1435
   
    Or the other TimedBuffer work? If not any ideas what this is, i haven't touched this area at all for this PR so doesn't make sense to me.
   
    Cheers
    Mike


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @michaelandrepearce try to see if you version include this: https://github.com/apache/activemq-artemis/commit/74f243cc4d9ec46f041e765148bdcb8be5af8b10
    It's a fix for that test


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @franz1981 i assume it would i thought PR builds merge with master, as such should pick up, I've forced it to rebuild by doing a whitespace push to see if its sporadic or not.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @franz1981 looks like its sporadic. just done two builds and it didn't occur again


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    PR builds don't "merge with master" in any kind of automatic way.  The PR build simply builds the PR.  It's up to the submitter to rebase on master if necessary.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Delegate acknowlegde to ClientMe...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    Have added a test case to ensure perf is at least double when many message, we expect it to be more as it should no longer block at all and it is, but was a good way to test its giving the improvement expected, e.g. the prior to the change or reverting the test fails as the perf is equal between the two states.
   
   
    Here is a local output of the improvement in consume time of 10,000 when we make acknowledge non-blocking.
   
    [main] 06:13:57,987 INFO  [org.apache.activemq.artemis.jms.tests] BlockOnAcknowledge=false MessageCount=10000 TimeToConsume=462980029
   
    [main] 06:13:57,987 INFO  [org.apache.activemq.artemis.jms.tests] BlockOnAcknowledge=true MessageCount=10000 TimeToConsume=38748489101


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @jbertram thanks for clarifying my incorrect assumption on how pr build works.
   
    Have as such took the opportunity to rebase, squash and correct the commit message to reflect now current solution.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @michaelandrepearce I've made a fix for that test here: https://github.com/apache/activemq-artemis/pull/1444


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @franz1981 thanks, we've got a succesfull build post my rebase, and squash anyhow :).
   
    @ all this i think is in a state to look to merge, tests all passing in PR build.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    There are 3 tests failures on org.apache.activemq.artemis.tests.integration.jms.consumer.JmsConsumerTest
   
    something around individual ack.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @clebertsuconic which profile i need to run on maven for that (obviously i can run just that test but if i look to fix anything i would want to rerun the whole suite/profile of tests)? locally I'm running the same build as the PR does atm.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    mvn test -Ptests
   
    it takes about 2 hours


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1428: ARTEMIS-1308: Make acknowlegde in AcitveMQMess...

franz1981
In reply to this post by franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1428
 
    @clebertsuconic thanks, ill run that.
   
    re the individual acks on that test, simple moved the close method before checking state in the test, as on close state is actual, before that with block on ack it is just eventual. have pushed this change.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...