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/06/02 08:46:40 UTC
Re: 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/
-----------------------------------------------------------
(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/#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
>
>