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