----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23545/#review47865 -----------------------------------------------------------
Looks good, a couple of minor suggestions though, the first of which is whether we could pick a better name for the new connection option. E.g. byte-capacity? That matches the use of capacity rather than prefetch in the receiver class. /trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp <https://reviews.apache.org/r/23545/#comment84101> I find this name a little awkward. Perhaps MINIMUM_FRAME_SIZE? It would also be nicer to declare it in the anonymous namespace. /trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp <https://reviews.apache.org/r/23545/#comment84100> The max frame size should be taken from the options if specified (and really that should be regardless of whether a byte capacity has been set). - Gordon Sim On July 16, 2014, 9:59 a.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23545/ > ----------------------------------------------------------- > > (Updated July 16, 2014, 9:59 a.m.) > > > Review request for qpid, Gordon Sim and mick goulish. > > > Bugs: QPID-5895 > https://issues.apache.org/jira/browse/QPID-5895 > > > Repository: qpid > > > Description > ------- > > AMQP 0-10: > byteCredit in 0-10 ReceiverImpl renamed to byteWindow. > > Both (message and byte) window/capacity operate independently on each other. > > I had to add SessionImpl::getMaxPrefetchedBytes to get around pre-declared > class Connection. > > > AMQP 1.0: > If max-prefetched-bytes to be used, I fixed maxFrameForBytePrefetch in 'open' > performative to 8k. That should be enough high for most of messages and > should provide sufficient granularity of incoming-window. > > Note, pn_session_set_incoming_capacity sets session's byte capacity whereas > incoming_window is returned by proton method: > > size_t pn_session_incoming_window(pn_session_t *ssn) > { > uint32_t size = ssn->connection->transport->local_max_frame; > if (!size) { > return 2147483647; // biggest legal value > } else { > return (ssn->incoming_capacity - ssn->incoming_bytes)/size; > } > } > > If a user sets max-prefetched-bytes below 8k, the client will automatically > increase it to 8k with some warning (otherwise incoming-window would be zero > and no consumer would get a single message). > > Note, the 1.0 patch does not work due to QPID-5896 I plan to fix first. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.h 1609704 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1609704 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.h 1609704 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp 1609704 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/SessionImpl.h 1609704 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/SessionImpl.cpp 1609704 > /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.h 1609704 > /trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1609704 > /trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp 1609704 > > Diff: https://reviews.apache.org/r/23545/diff/ > > > Testing > ------- > > Automated tests passed (well, TestParseTcpIPv6 failed but that fails on trunk > as well - already reported). > > Manual tests on 0-10 passed as well. On 1.0, they would work (per tcpdump, > both max-frame-size in 'open' performative and incoming-window in 'begin'), > but only after QPID-5896 is fixed :-/ . > > (I will post the patch only after QPID-5896 is fixed (and after ACKs from > this review, of course :) ) > > > Thanks, > > Pavel Moravec > >
