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