You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by ganeshmurthy <gi...@git.apache.org> on 2018/06/22 20:03:40 UTC

[GitHub] qpid-dispatch pull request #328: DISPATCH-1045 - Release the delivery only a...

GitHub user ganeshmurthy opened a pull request:

    https://github.com/apache/qpid-dispatch/pull/328

    DISPATCH-1045 - Release the delivery only after the entire message ha…

    …s been received

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

    $ git pull https://github.com/ganeshmurthy/qpid-dispatch DISPATCH-1045

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

    https://github.com/apache/qpid-dispatch/pull/328.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 #328
    
----
commit e40858d7da687e427d624409ba60cb27c1fe0550
Author: Ganesh Murthy <gm...@...>
Date:   2018-06-22T20:01:51Z

    DISPATCH-1045 - Release the delivery only after the entire message has been received

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


[GitHub] qpid-dispatch issue #328: DISPATCH-1045 - Release the delivery only after th...

Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:

    https://github.com/apache/qpid-dispatch/pull/328
  
    # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=h1) Report
    > Merging [#328](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=desc) into [master](https://codecov.io/gh/apache/qpid-dispatch/commit/2e766f35cf2b3a172aaa58cef6c996376e69f1c9?src=pr&el=desc) will **increase** coverage by `0.1%`.
    > The diff coverage is `92.85%`.
    
    [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/328/graphs/tree.svg?src=pr&token=rk2Cgd27pP&width=650&height=150)](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=tree)
    
    ```diff
    @@            Coverage Diff            @@
    ##           master     #328     +/-   ##
    =========================================
    + Coverage   86.52%   86.63%   +0.1%     
    =========================================
      Files          69       69             
      Lines       15479    15494     +15     
    =========================================
    + Hits        13393    13423     +30     
    + Misses       2086     2071     -15
    ```
    
    
    | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=tree) | Coverage Δ | |
    |---|---|---|
    | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/328/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `94.49% <100%> (+0.06%)` | :arrow_up: |
    | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/328/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `89.96% <80%> (-0.05%)` | :arrow_down: |
    | [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/328/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `99.25% <0%> (+0.37%)` | :arrow_up: |
    | [src/router\_core/agent\_link.c](https://codecov.io/gh/apache/qpid-dispatch/pull/328/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL2FnZW50X2xpbmsuYw==) | `63.79% <0%> (+0.57%)` | :arrow_up: |
    | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/328/diff?src=pr&el=tree#diff-c3JjL21lc3NhZ2UuYw==) | `87.34% <0%> (+1.48%)` | :arrow_up: |
    
    ------
    
    [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=continue).
    > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
    > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
    > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=footer). Last update [2e766f3...e40858d](https://codecov.io/gh/apache/qpid-dispatch/pull/328?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).



---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


[GitHub] qpid-dispatch pull request #328: DISPATCH-1045 - Release the delivery only a...

Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on a diff in the pull request:

    https://github.com/apache/qpid-dispatch/pull/328#discussion_r198613623
  
    --- Diff: src/router_node.c ---
    @@ -312,12 +307,30 @@ static void AMQP_rx_handler(void* context, qd_link_t *link)
                            pn_link_name(pn_link));
             }
     
    +        //
    +        // The entire message has been received and we are ready to consume the delivery by calling pn_link_advance().
    +        //
    +        pn_link_advance(pn_link);
    +
    +        //
    +        // The entire message has been received but this message needs to be discarded
    +        //
    +        if (qd_message_is_discard(msg)) {
    +            pn_delivery_update(pnd, qdr_delivery_disposition(delivery));
    --- End diff --
    
    Agreed. I have pushed up a new commit to remedy your comment. Please take a quick look. Thanks.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


[GitHub] qpid-dispatch pull request #328: DISPATCH-1045 - Release the delivery only a...

Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:

    https://github.com/apache/qpid-dispatch/pull/328#discussion_r198605509
  
    --- Diff: src/router_node.c ---
    @@ -312,12 +307,30 @@ static void AMQP_rx_handler(void* context, qd_link_t *link)
                            pn_link_name(pn_link));
             }
     
    +        //
    +        // The entire message has been received and we are ready to consume the delivery by calling pn_link_advance().
    +        //
    +        pn_link_advance(pn_link);
    +
    +        //
    +        // The entire message has been received but this message needs to be discarded
    +        //
    +        if (qd_message_is_discard(msg)) {
    +            pn_delivery_update(pnd, qdr_delivery_disposition(delivery));
    --- End diff --
    
    Is the delivery disposition _always_ set when discard is set?  What if the disposition was not set?  I think the pn_delivery_update should not be called in this case.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org