You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by michael goulish <mg...@redhat.com> on 2015/05/01 19:34:18 UTC
Review Request 33758: do something reasonable with local and remote
max channels
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/
-----------------------------------------------------------
Review request for qpid, Kenneth Giusti and Ted Ross.
Repository: qpid-proton-git
Description
-------
PROTON-842 -- channels and sessions
Diffs
-----
proton-c/src/engine/engine-internal.h e5ec602
proton-c/src/engine/engine.c 5e05cbc
proton-c/src/framing/framing.h 9650979
proton-c/src/transport/transport.c 62d4742
Diff: https://reviews.apache.org/r/33758/diff/
Testing
-------
tested with modified simple_send.py and reactor.py
and qdrouterd.
my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
Each simple_sender makes 200 links over a single connection, to router B.
These become link-routes through router A to the broker.
the purpose of this diff is to get proton code to
-------------------------------------------------------
1. not cause router to crash when channels go above 2^15
2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
Thanks,
michael goulish
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
> On May 2, 2015, 10:29 a.m., Rafael Schloming wrote:
> > proton-c/src/transport/transport.c, line 1110
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line1110>
> >
> > This shouldn't close at all, pn_do_error will send a close frame if necessary. This call is mutating the local top-half endpoint state which makes no sense here.
Thanks very much! I yanked that.
- michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82315
-----------------------------------------------------------
On May 1, 2015, 5:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 5:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82315
-----------------------------------------------------------
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment133046>
This shouldn't close at all, pn_do_error will send a close frame if necessary. This call is mutating the local top-half endpoint state which makes no sense here.
- Rafael Schloming
On May 1, 2015, 5:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 5:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/engine/engine-internal.h, line 192
> > <https://reviews.apache.org/r/33758/diff/1/?file=947288#file947288line192>
> >
> > Why three values of channel_max? It seems redundant. Is "channel_max" now just a convenient place to store the minimum?
Yes, I just thought it would be nice to have places to explicitly store the local coding constraint and the remote peer constraint separately. But I don't have any strong reason for it. Debugging, or something vague like that. And it seemed like a small cost. You think I should just collapse this to one value? Say the word, and I will.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/engine/engine-internal.h, line 211
> > <https://reviews.apache.org/r/33758/diff/1/?file=947288#file947288line211>
> >
> > The comment is unhelpful.
> >
> > Isn't channel zero valid?
Yes, sorry. I actually removed this entirely -- instead I put explicit failure codes in allocate_alias() and its callers.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/engine/engine.c, line 936
> > <https://reviews.apache.org/r/33758/diff/1/?file=947289#file947289line936>
> >
> > What will happen if channel_max is zero? This seems to be the default. I think this >= comparison will be incorrect.
> >
> > Does the size of the local_channels hash get set based on the connection-peer's channel-max?
I realized I should initialize channel_max to local_channel_max. The 'size' of the hash is how may things it's storing. It also has a 'capacity', but I am not bothering to reduce that based on these constraints. hmmm.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/transport/transport.c, line 377
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line377>
> >
> > Why not 32767?
Yes, sorry, I will change this. I did that because I was having trouble actually getting my test to make that many links. Some of the clients just .. never getting through, somehow.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/transport/transport.c, line 1036
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line1036>
> >
> > If the peer says his channel-max is zero, I would assume the channel-max is, in fact, zero.
> >
> > Don't confuse channel-max with "max channels".
OK, I'll change that. B-but ... I dont understand the distinction you're making. Wouldn't this be the peer saying that we cannot make any sessions?
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/transport/transport.c, line 1040
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line1040>
> >
> > Nit-pick. You are using a style (whitespace, position of open bracket) that is different from the code you are modifying.
ok, i will change. this style makes it easier for me to see.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/transport/transport.c, line 1051
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line1051>
> >
> > The asserts are invalid as zero is a valid channel-max.
removed.
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/transport/transport.c, line 2594
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line2594>
> >
> > The logic here may be flawed (depending on the meaning of ->channel_max). For example, If I call this function and set the max to 10, then call again to set it to 20. It will stay at 10.
> >
> > Shouldn't this function just set the local_channel_max and, if needed, recompute channel_max?
local_channel_max is imposed by the code implementation itself. I can't let the app override that. I could do what you are saying -- let the app change its mind higher -- if I store Yet One More channel-max variable, to remember what the app said , and then take the minimum of ( app's limit, local-coding-limit, peer limit ). To me, that didn't seem worth it... But I'm happy to do it if that seems best to you. Say the word!
> On May 1, 2015, 6:34 p.m., Ted Ross wrote:
> > proton-c/src/framing/framing.h, line 35
> > <https://reviews.apache.org/r/33758/diff/1/?file=947290#file947290line35>
> >
> > A bit of a nit-pick. This should be called AMQP_DEFAULT_CHANNEL_MAX. The current name doesn't match the spec and suggests that the maximum number of channels is 65535.
That's not a nit-pick! I want it to be right.
Oops -- I ended up having no use for this, so yanked it.
- michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82264
-----------------------------------------------------------
On May 1, 2015, 5:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 5:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Ted Ross <tr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82264
-----------------------------------------------------------
proton-c/src/engine/engine-internal.h
<https://reviews.apache.org/r/33758/#comment132983>
Why three values of channel_max? It seems redundant. Is "channel_max" now just a convenient place to store the minimum?
proton-c/src/engine/engine-internal.h
<https://reviews.apache.org/r/33758/#comment132967>
The comment is unhelpful.
Isn't channel zero valid?
proton-c/src/engine/engine.c
<https://reviews.apache.org/r/33758/#comment132965>
What will happen if channel_max is zero? This seems to be the default. I think this >= comparison will be incorrect.
Does the size of the local_channels hash get set based on the connection-peer's channel-max?
proton-c/src/framing/framing.h
<https://reviews.apache.org/r/33758/#comment132969>
A bit of a nit-pick. This should be called AMQP_DEFAULT_CHANNEL_MAX. The current name doesn't match the spec and suggests that the maximum number of channels is 65535.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132971>
Why not 32767?
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132973>
If the peer says his channel-max is zero, I would assume the channel-max is, in fact, zero.
Don't confuse channel-max with "max channels".
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132972>
Nit-pick. You are using a style (whitespace, position of open bracket) that is different from the code you are modifying.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132974>
The asserts are invalid as zero is a valid channel-max.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132978>
Will this close have a framing-error in its error field?
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132984>
The logic here may be flawed (depending on the meaning of ->channel_max). For example, If I call this function and set the max to 10, then call again to set it to 20. It will stay at 10.
Shouldn't this function just set the local_channel_max and, if needed, recompute channel_max?
- Ted Ross
On May 1, 2015, 1:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 1:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
> On May 1, 2015, 6:25 p.m., Kenneth Giusti wrote:
> > proton-c/src/transport/transport.c, line 1849
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line1849>
> >
> > Could the channel be invalid? If so, should that also cause the setup to fail?
I'm adding an explicit 'invalid' return value to pni_map_local_channel, and failing the setup if it fails.
> On May 1, 2015, 6:25 p.m., Kenneth Giusti wrote:
> > proton-c/src/transport/transport.c, line 2608
> > <https://reviews.apache.org/r/33758/diff/1/?file=947291#file947291line2608>
> >
> > This statement has no effect: you don't constrain to local_channel_max in the > case.
I realized it makes sense to initialize channel_max to same as local_channel_max -- so when (if) this call comes in, channel_max will already have a valid value. So this call from the app should only be able to further contrain the value. changed it that way.
- michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82265
-----------------------------------------------------------
On May 1, 2015, 5:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 5:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82265
-----------------------------------------------------------
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132970>
Could the channel be invalid? If so, should that also cause the setup to fail?
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment132979>
This statement has no effect: you don't constrain to local_channel_max in the > case.
- Kenneth Giusti
On May 1, 2015, 5:34 p.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 5:34 p.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/framing/framing.h 9650979
> proton-c/src/transport/transport.c 62d4742
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Ted Ross <tr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review82508
-----------------------------------------------------------
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment133252>
If channel-max is zero, that means the highest allowed channel number is zero. It permits exactly one session (session zero). There isn't a way to ask for zero channels. That wouldn't be very useful.
- Ted Ross
On May 5, 2015, 4:18 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 4:18 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/include/proton/transport.h 690952b
> proton-c/src/engine/engine-internal.h e5ec602
> proton-c/src/engine/engine.c 5e05cbc
> proton-c/src/transport/transport.c 62d4742
> tests/python/proton_tests/engine.py 82869db
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> Note for new diff --
>
> It is still possible for applications -- like dispatch -- to crash, since I haven't done anything to change them yet. But we no longer crash in Proton when we exceed 2^15 sessions in one connection.
>
> In this diff I added changes to allocate_alias and its callers to explicitly return an invalid result.
> The one test that was failing was due to a teensy mistake in the test.
>
>
>
> -------------------------------------------------------------
> -
> -Notes for previous diff:
> -
>
> tested with modified simple_send.py and reactor.py
> and qdrouterd.
>
> my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
>
> Each simple_sender makes 200 links over a single connection, to router B.
> These become link-routes through router A to the broker.
>
> the purpose of this diff is to get proton code to
> -------------------------------------------------------
> 1. not cause router to crash when channels go above 2^15
> 2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
>
>
> I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
>
> Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review86217
-----------------------------------------------------------
Ship it!
Ship It!
- michael goulish
On June 2, 2015, 6:46 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 6:46 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/include/proton/transport.h d046567
> proton-c/src/engine/engine-internal.h 2f0cc56
> proton-c/src/engine/engine.c 67cc882
> proton-c/src/transport/transport.c e72875b
> tests/python/proton_tests/engine.py 924b3bc
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
>
> This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review87066
-----------------------------------------------------------
proton-c/src/engine/engine.c
<https://reviews.apache.org/r/33758/#comment139337>
The total number of _active_ sessions can be 1 greater than the value of channel_max. channel # can range from 0 thru channel_max.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment139343>
We may be off by one here, since the valid range of channels starts at zero and includes channel_max (limit in this code). So you want to check <= limit.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment139335>
May want to burp out a log message here so users understand why the setup failed.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment139344>
is this part of the link handle change?
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment139334>
still 65536
tests/python/proton_tests/engine.py
<https://reviews.apache.org/r/33758/#comment139345>
You should add a test that sets channel_max to some low number (say 1) and verify you can open [0..channel_max] sessions.
- Kenneth Giusti
On June 2, 2015, 6:46 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 6:46 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/include/proton/transport.h d046567
> proton-c/src/engine/engine-internal.h 2f0cc56
> proton-c/src/engine/engine.c 67cc882
> proton-c/src/transport/transport.c e72875b
> tests/python/proton_tests/engine.py 924b3bc
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
>
> This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Kenneth Giusti <kg...@apache.org>.
> On June 17, 2015, 3:05 p.m., Kenneth Giusti wrote:
> > proton-c/bindings/python/proton/__init__.py, line 2489
> > <https://reviews.apache.org/r/33758/diff/4/?file=986235#file986235line2489>
> >
> > In python, it would be better to raise an exception here instead of returning None.
> >
> > The following code should work, but is totally untested :)
> >
> > if ssn is None:
> > raise(SessionException("Session allocation failed --- blah blah blah")
> >
> >
> >
> > you'll also have to export SessionException by adding it to the __all__ array in that file.
damn markdown - that should be "__all__" - the array has double leading and trailing underscores in it's name.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review88227
-----------------------------------------------------------
On June 17, 2015, 5:45 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 5:45 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/bindings/python/proton/__init__.py 9432bd8
> proton-c/include/proton/cproton.i ac2b121
> proton-c/include/proton/transport.h a3ca667
> proton-c/src/engine/engine-internal.h 4c72310
> proton-c/src/engine/engine.c c5228a5
> proton-c/src/transport/transport.c 0e23975
> tests/python/proton_tests/engine.py 924b3bc
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
>
> This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review88227
-----------------------------------------------------------
tests/python/proton_tests/engine.py (line 1279)
<https://reviews.apache.org/r/33758/#comment140636>
Remove this
tests/python/proton_tests/engine.py (line 2279)
<https://reviews.apache.org/r/33758/#comment140637>
remove this
proton-c/bindings/python/proton/__init__.py (line 2489)
<https://reviews.apache.org/r/33758/#comment140642>
In python, it would be better to raise an exception here instead of returning None.
The following code should work, but is totally untested :)
if ssn is None:
raise(SessionException("Session allocation failed --- blah blah blah")
you'll also have to export SessionException by adding it to the __all__ array in that file.
proton-c/include/proton/transport.h (line 335)
<https://reviews.apache.org/r/33758/#comment140635>
Rephrase to clarify:
"Note that this is the maximum channel number allowed, giving a valid channel number range of [0..channel_max]. Therefore the maximum number of simultaineously active channels will be channel_max plus 1"
tests/python/proton_tests/engine.py (line 254)
<https://reviews.apache.org/r/33758/#comment140643>
you can test for the SessionException here:
try:
ssn_1 = self.c2.session()
assert False, "expected session exception"
exception SessionException:
pass
again, totally untested...
tests/python/proton_tests/engine.py (line 1279)
<https://reviews.apache.org/r/33758/#comment140640>
remove this
tests/python/proton_tests/engine.py (line 2279)
<https://reviews.apache.org/r/33758/#comment140639>
Remove this
- Kenneth Giusti
On June 17, 2015, 5:45 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 5:45 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/bindings/python/proton/__init__.py 9432bd8
> proton-c/include/proton/cproton.i ac2b121
> proton-c/include/proton/transport.h a3ca667
> proton-c/src/engine/engine-internal.h 4c72310
> proton-c/src/engine/engine.c c5228a5
> proton-c/src/transport/transport.c 0e23975
> tests/python/proton_tests/engine.py 924b3bc
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
>
> This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/
-----------------------------------------------------------
(Updated June 17, 2015, 5:45 a.m.)
Review request for qpid, Kenneth Giusti and Ted Ross.
Changes
-------
added unit test to ensure that channel_max really limits session creation. This required a change to swig file to allow pn_session to return NULL.
Repository: qpid-proton-git
Description
-------
PROTON-842 -- channels and sessions
Diffs (updated)
-----
proton-c/bindings/python/proton/__init__.py 9432bd8
proton-c/include/proton/cproton.i ac2b121
proton-c/include/proton/transport.h a3ca667
proton-c/src/engine/engine-internal.h 4c72310
proton-c/src/engine/engine.c c5228a5
proton-c/src/transport/transport.c 0e23975
tests/python/proton_tests/engine.py 924b3bc
Diff: https://reviews.apache.org/r/33758/diff/
Testing
-------
I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
Thanks,
michael goulish
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/#review86212
-----------------------------------------------------------
Looks good. Pretty clear what the fix is doing.
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment138134>
Avoid first person pronouns in error messages. Maybe "remote channel %d is above negotiated channel_max %d."
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment138135>
Next line is already 65536. Stale comment?
proton-c/src/transport/transport.c
<https://reviews.apache.org/r/33758/#comment138136>
s/below/above/
- Chug Rolke
On June 2, 2015, 6:46 a.m., michael goulish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33758/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 6:46 a.m.)
>
>
> Review request for qpid, Kenneth Giusti and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-842 -- channels and sessions
>
>
> Diffs
> -----
>
> proton-c/include/proton/transport.h d046567
> proton-c/src/engine/engine-internal.h 2f0cc56
> proton-c/src/engine/engine.c 67cc882
> proton-c/src/transport/transport.c e72875b
> tests/python/proton_tests/engine.py 924b3bc
>
> Diff: https://reviews.apache.org/r/33758/diff/
>
>
> Testing
> -------
>
> I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
>
> This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
>
>
> Thanks,
>
> michael goulish
>
>
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/
-----------------------------------------------------------
(Updated June 2, 2015, 6:46 a.m.)
Review request for qpid, Kenneth Giusti and Ted Ross.
Changes
-------
This is quite different from what I had a while ago. I think it's more clear (and more correct) -- and it has some unit tests.
Repository: qpid-proton-git
Description
-------
PROTON-842 -- channels and sessions
Diffs (updated)
-----
proton-c/include/proton/transport.h d046567
proton-c/src/engine/engine-internal.h 2f0cc56
proton-c/src/engine/engine.c 67cc882
proton-c/src/transport/transport.c e72875b
tests/python/proton_tests/engine.py 924b3bc
Diff: https://reviews.apache.org/r/33758/diff/
Testing (updated)
-------
I originally did large system-testing using a broker and dispatch routers to get 32K links through a single connection.
This diff is based on what I did earlier, but improved (i think), and with a couple mistakes corrected. Now I am testing only with proton unit tests included with this diff, because latest dispatch is having an issue with latest proton.
Thanks,
michael goulish
Re: Review Request 33758: do something reasonable with local and
remote max channels
Posted by michael goulish <mg...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33758/
-----------------------------------------------------------
(Updated May 5, 2015, 8:18 a.m.)
Review request for qpid, Kenneth Giusti and Ted Ross.
Changes
-------
PROTON-842 -- channels and sessions -- version 2 -- return invalid result from allocate_alias
Repository: qpid-proton-git
Description
-------
PROTON-842 -- channels and sessions
Diffs (updated)
-----
proton-c/include/proton/transport.h 690952b
proton-c/src/engine/engine-internal.h e5ec602
proton-c/src/engine/engine.c 5e05cbc
proton-c/src/transport/transport.c 62d4742
tests/python/proton_tests/engine.py 82869db
Diff: https://reviews.apache.org/r/33758/diff/
Testing (updated)
-------
Note for new diff --
It is still possible for applications -- like dispatch -- to crash, since I haven't done anything to change them yet. But we no longer crash in Proton when we exceed 2^15 sessions in one connection.
In this diff I added changes to allocate_alias and its callers to explicitly return an invalid result.
The one test that was failing was due to a teensy mistake in the test.
-------------------------------------------------------------
-
-Notes for previous diff:
-
tested with modified simple_send.py and reactor.py
and qdrouterd.
my script has 1 qpidd broker, 2 routers, and 200 simple_senderer.
Each simple_sender makes 200 links over a single connection, to router B.
These become link-routes through router A to the broker.
the purpose of this diff is to get proton code to
-------------------------------------------------------
1. not cause router to crash when channels go above 2^15
2. do something reasonable in this case, so that application level has a chance of doing something reasonable.
I am not doing handles for links yet -- I want to get review for this first, get this done, and then do same thing there. I expect those changes will be identical.
Also please note -- I did NOT try to quit using the top bit of channel number as a flag. Just advertising a lower number, trying to do something reasonable wrt local and remote max channels, and trying to honor what the other side says.
Thanks,
michael goulish