----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51638/#review147907 -----------------------------------------------------------
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. src/qpid/messaging/ConnectionOptions.cpp (line 56) <https://reviews.apache.org/r/51638/#comment215201> 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) src/qpid/messaging/amqp/ConnectionContext.cpp (line 205) <https://reviews.apache.org/r/51638/#comment215202> Message should say why: "could not sync session because a message was rejected" src/qpid/messaging/amqp/EncodedMessage.cpp (line 54) <https://reviews.apache.org/r/51638/#comment215203> Why memmove? Is it legal for two EncodedMessages to have overlapping buffers? If not this might be clearer: if (data != other.data) memcpy(...) src/qpid/messaging/amqp/SenderContext.cpp (line 152) <https://reviews.apache.org/r/51638/#comment215211> 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. src/qpid/messaging/amqp/SenderContext.cpp (line 165) <https://reviews.apache.org/r/51638/#comment215207> This looks right but the user has no idea it is happening. Often that is good, but do we need a way for people to know? We had the same problem with excessivly transparent reconnects before. src/qpid/messaging/amqp/SenderContext.cpp (line 181) <https://reviews.apache.org/r/51638/#comment215212> If we get here there are no deliveries left, so even if there were some needing resend they're gone now. src/qpid/messaging/amqp/SenderContext.cpp (line 585) <https://reviews.apache.org/r/51638/#comment215214> 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. src/qpid/messaging/amqp/SenderContext.cpp (line 606) <https://reviews.apache.org/r/51638/#comment215209> Maybe "name:description", the name might be important. src/qpid/messaging/amqp/SenderContext.cpp (line 650) <https://reviews.apache.org/r/51638/#comment215205> 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. - Alan Conway 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 > >
