You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2014/07/16 11:59:00 UTC
Review Request 23545: [C++ client] Allow consumer flow control based on
#bytes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23545/
-----------------------------------------------------------
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
Re: Review Request 23545: [C++ client] Allow consumer flow control based on
#bytes
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23545/
-----------------------------------------------------------
(Updated July 16, 2014, 1:12 p.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 (updated)
-----
/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
Re: Review Request 23545: [C++ client] Allow consumer flow control based on
#bytes
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 23545: [C++ client] Allow consumer flow control based on
#bytes
Posted by Pavel Moravec <pm...@redhat.com>.
> On July 16, 2014, 12:40 p.m., mick goulish wrote:
> > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp, line 45
> > <https://reviews.apache.org/r/23545/diff/2/?file=633436#file633436line45>
> >
> > I don't understand the "/2" parts here. Maybe a comment?
That has been there ever: re-issue credit, if available window drops below 1/2 of capacity.
Comment added.
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23545/#review47870
-----------------------------------------------------------
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
>
>
Re: Review Request 23545: [C++ client] Allow consumer flow control based on
#bytes
Posted by mick goulish <mi...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23545/#review47870
-----------------------------------------------------------
Ship it!
/trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp
<https://reviews.apache.org/r/23545/#comment84102>
I don't understand the "/2" parts here. Maybe a comment?
- mick goulish
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
>
>