qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "michael goulish" <mgoul...@redhat.com>
Subject Re: Review Request 33758: do something reasonable with local and remote max channels
Date Tue, 05 May 2015 08:12:04 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message