> On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > Mostly nits, one major problem: I see no way to control the frequency or > > limit the number of resends. We need the same kind of controls for that as > > we have for connection retries, otherwise we will burn CPU on client and > > router with constant resends if the back end takes time to recover, or > > never recovers, or keeps releasing the same messages. > > Alan Conway wrote: > Thought: for handling rejection and allowing manual handling of relase: > can we throw an exception that carries the message when handling the > disposition? At that point the user can do what they like: resend, not > resend, send somewhere else etc. etc. When automatic resend is disabled, then > I see no difference between release and reject: they're both things the user > might need to know about, and in both cases the user may want to do something > else with the message.
It would require a new exception type. That's not a big change to the API, but I was hoping to avoid it. We already have a MessageRejected exception, which is currently used only when doing sync send. I just used that same exception. Also at the point we get the rejection, what we have is only the encoded for of the message. Again, it's not a big deal to recreate a message object from that. I was just looking for a minimalist solution in the first instance. Thanks for all the great feedback. I'll post another update once I address the issues you raise. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/SenderContext.cpp, line 152 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492016#file1492016line152> > > > > Potential infinite loop? We push_back resends on the deliveries list, > > so it may never become empty. > > > > Aside from this loop we can get into a bigger infinite loop with a > > broker that keeps releasing the same message. We need configurable min/max > > timeouts, max retries etc. In the loop, if delivered() returns false, we break out the loop. I agree that continually released messages are a potential issue. I say potential, because at present the only thing I am aware of that would release messages is the router, and it would not do so infinitely. However I could probably add a max resend count without too much difficulty. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/SenderContext.cpp, line 181 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492016#file1492016line181> > > > > If we get here there are no deliveries left, so even if there were some > > needing resend they're gone now. No, because we exit the loop once we get to the first unconfirmed message. Messages that have been marked for resend will always be undelivered at this point, and so will still be on the queue. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/SenderContext.cpp, line 586 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492016#file1492016line586> > > > > Too many warning logs. In a client lib any info that warrants a warning > > needs to be returned via the API somehow so the user can deal with it. Text > > in a log file is not useful to a running client. The warnings aren't new. The current behaviour is just to log the warning and otherwise ignore things. That behaviour will still be available with the appropriate configuration. While I do take your point about the ideal solution, I want to improve on the current situation without changing the API and I feel in this case that some logging is necessary. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/SenderContext.cpp, line 607 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492016#file1492016line607> > > > > Maybe "name:description", the name might be important. Agreed, will update. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/SenderContext.cpp, line 651 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492016#file1492016line651> > > > > I don't understand. Any modifed message is not delivered, regardless of > > whether it is marked delivery_failed - by my reading there is effectively > > no difference betwee modified with no fields set and released. You're right of course. I'll change that. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/ConnectionOptions.cpp, line 56 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492012#file1492012line56> > > > > Nit, I don't like double negatives. What about retryReleased(true), > > raiseRefused(true)? To me "do X" is clearer than "don't do nothing (and X > > is implied but not stated) I'll switch to your suggestions. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/ConnectionContext.cpp, line 205 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492013#file1492013line205> > > > > Message should say why: "could not sync session because a message was > > rejected" The exception will indicate the rejection error, but yes, I will make this a bit clearer. > On Sept. 6, 2016, 8:18 p.m., Alan Conway wrote: > > src/qpid/messaging/amqp/EncodedMessage.cpp, line 54 > > <https://reviews.apache.org/r/51638/diff/3/?file=1492014#file1492014line54> > > > > Why memmove? Is it legal for two EncodedMessages to have overlapping > > buffers? If not this might be clearer: > > > > if (data != other.data) memcpy(...) Agreed, will fix. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51638/#review147907 ----------------------------------------------------------- On Sept. 6, 2016, 4:53 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51638/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2016, 4:53 p.m.) > > > Review request for qpid and Alan Conway. > > > Repository: qpid-cpp > > > Description > ------- > > Rejected messages cause an exception to be thrown. However this does not > invalidate the session in anyway. More messages can be sent after > catching the exception. The original behaviour - i.e. simply ignoring > the rejected messages - can be obtained by setting the connection option > 'ignore_delivery_refused' to true. > > Released messages cause the transport to be aborted, triggering the usual > resending logic (whether defined by the application or using that defined > in the library itself). Again, released messages can be simply ignored as > they were prior to this change by setting 'ignore_undelivered' to true. > > For modified messages, if undeliverable-here is set to true, the message > is > treated as if it had been rejected, otheriwse if delivery-failed is set to > true it is treated as a released message. If neither is set it is simply > ignored with a warning (i.e. treated as accepted). > > > Diffs > ----- > > src/qpid/messaging/ConnectionOptions.h c8c8798 > src/qpid/messaging/ConnectionOptions.cpp d956e9a > src/qpid/messaging/amqp/ConnectionContext.cpp 25dd68d > src/qpid/messaging/amqp/EncodedMessage.cpp cf60046 > src/qpid/messaging/amqp/SenderContext.h 467a8e0 > src/qpid/messaging/amqp/SenderContext.cpp fe8b4d3 > src/qpid/messaging/amqp/SessionContext.h 67b3c1e > src/qpid/messaging/amqp/SessionContext.cpp 92bdea7 > src/qpid/messaging/amqp/Transaction.cpp 754b00d > > Diff: https://reviews.apache.org/r/51638/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
