You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by mbroadst <gi...@git.apache.org> on 2016/06/23 13:43:38 UTC
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
GitHub user mbroadst opened a pull request:
https://github.com/apache/qpid/pull/9
feat(disposition): support undeliverable-here in modified outcomes
Previously, specifying `undeliverable-here` as `true` in a modified
outcome simply resulted in the message being rejected. This patch
adds tracking to the outgoing link management in order to not
redeliver messages to links that indicate that messages are not
deliverable there.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mbroadst/qpid support-undeliverable-here
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid/pull/9.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 #9
----
commit 76d635bc602ecbe66be01d8747b796fc0ce541de
Author: Matt Broadstone <mb...@gmail.com>
Date: 2016-06-23T13:41:57Z
feat(disposition): support undeliverable-here in modified outcomes
Previously, specifying `undeliverable-here` as `true` in a modified
outcome simply resulted in the message being rejected. This patch
adds tracking to the outgoing link management in order to not
redeliver messages to links that indicate that messages are not
deliverable there.
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on the issue:
https://github.com/apache/qpid/pull/9
@grs here is the amqp10-based test case:
```
'use strict';
const amqp = require('amqp10'),
Policy = amqp.Policy,
expect = require('chai').expect,
config = { address: 'localhost' };
let test = {};
describe('modified', () => {
afterEach(() => test.client.disconnect().then(() => { delete test.client; }));
it('should respect undeliverable here', function(done) {
test.client = new amqp.Client(Policy.Utils.RenewOnSettle(1, 1, Policy.Default));
return test.client.connect(config.address)
.then(() => Promise.all([
test.client.createSender('test.disposition.queue'),
test.client.createReceiver('test.disposition.queue')]))
.spread((sender, receiver) => {
let received = 0;
receiver.on('message', msg => {
expect(received).to.eql(0);
received++;
receiver.modify(msg, { undeliverableHere: true });
return test.client.createReceiver('test.disposition.queue')
.then(receiver2 => {
receiver2.on('message', msg => {
expect(received).to.eql(1);
done();
});
});
});
return sender.send('testing');
});
});
});
```
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68313162
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -142,11 +145,10 @@ void OutgoingFromQueue::handle(pn_delivery_t* delivery)
if (preAcquires()) {
//TODO: handle message-annotations
if (pn_disposition_is_undeliverable(pn_delivery_remote(delivery))) {
- //treat undeliverable here as rejection
- queue->reject(r.cursor);
- } else {
- queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
+ undeliverableMessages.add(r.msg.getSequence());
}
+
+ queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
--- End diff --
To be fair I think this reverts to the behavior before your patch, correct? As in this isn't breaking any already held expectations. Having said that, of course this might have been undesired behavior from the get go.
I'm hesitant to add per-queue configuration as I'm not quite sure how I would specify the with amqp 1.0 (iirc most of the create metadata is 0-10?), but am totally willing to explore those options. The broker-level option seems more digestible to me 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst closed the pull request at:
https://github.com/apache/qpid/pull/9
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on the issue:
https://github.com/apache/qpid/pull/9
@grs no worries, it's the functionality I'm after not the attribution \U0001f604
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68322398
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
Either of those works. One other point is that we need to deregister the observer when the consumer ends.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68299289
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
If possible, I'd really like to make the tracking lazy, only activating if actually requested/needed, and only then registering the queue observer and tracking the messages.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on the issue:
https://github.com/apache/qpid/pull/9
Actually, I've confirmed a patch isn't necessary. I can use your PR branch. I'll give this a test today and all going well merge it in.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on the issue:
https://github.com/apache/qpid/pull/9
@grs sure thing
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68328482
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
one more question here: is the right place to stop observation in `detached`?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68312642
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
sure that sounds reasonable. you think its enough to just register it during `handle` below, with a boolean flag on the class?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68299958
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -142,11 +145,10 @@ void OutgoingFromQueue::handle(pn_delivery_t* delivery)
if (preAcquires()) {
//TODO: handle message-annotations
if (pn_disposition_is_undeliverable(pn_delivery_remote(delivery))) {
- //treat undeliverable here as rejection
- queue->reject(r.cursor);
- } else {
- queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
+ undeliverableMessages.add(r.msg.getSequence());
}
+
+ queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
--- End diff --
One issue here is that the message will remain on the queue, even if there are no other consumers. In some cases that may be what you want, in others though it could cause a problem.
What would you think of having a queue level setting that dictated whether consumers should reject or track undelivered? Ideally we would then have reject as the default for now for backwards compatibility, but the behaviour you desire could be enabled when creating the queue? Alternatively we could even make the default a broker level option.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68322267
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -142,11 +145,10 @@ void OutgoingFromQueue::handle(pn_delivery_t* delivery)
if (preAcquires()) {
//TODO: handle message-annotations
if (pn_disposition_is_undeliverable(pn_delivery_remote(delivery))) {
- //treat undeliverable here as rejection
- queue->reject(r.cursor);
- } else {
- queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
+ undeliverableMessages.add(r.msg.getSequence());
}
+
+ queue->release(r.cursor, pn_disposition_is_failed(pn_delivery_remote(delivery)));
--- End diff --
That's a fair point. It wouldn't actually be a regression. Let's leave any queue options as a separate concern for now 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on the issue:
https://github.com/apache/qpid/pull/9
@mbroadst this is now committed. I somehow messed up the intended merge commit (I think perhaps on git svn rebase), so I'm afraid I neither marked the JIRA for this (https://issues.apache.org/jira/browse/QPID-7324) nor kept your name in the commit log. Thanks very much for the contribution though!
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid issue #9: feat(disposition): support undeliverable-here in modified out...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on the issue:
https://github.com/apache/qpid/pull/9
@grs okay I think that should take care of your comments from yesterday
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68367813
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
Yes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid pull request #9: feat(disposition): support undeliverable-here in modif...
Posted by mbroadst <gi...@git.apache.org>.
Github user mbroadst commented on a diff in the pull request:
https://github.com/apache/qpid/pull/9#discussion_r68322985
--- Diff: qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp ---
@@ -79,6 +79,9 @@ OutgoingFromQueue::OutgoingFromQueue(Broker& broker, const std::string& source,
void OutgoingFromQueue::init()
{
+ // observe queue for changes to track undeliverable messages
+ queue->getObservers().add(boost::shared_ptr<QueueObserver>(shared_from_this()));
+
--- End diff --
I wondered about that. From other examples of QueueObserver's in the codebase I didn't see anything that explicitly removed itself from the observer list, so assumed (dangerously/lazily \U0001f604) it was cleaned up automatically somewhere.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org