----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18423/#review35385 -----------------------------------------------------------
I think the change would be smaller (without as far as I can see any loss of function) by avoiding introducing the Optional template here. (There is already a similar template in use for QueueDepth where setting the size to 0 explicitly overrides the default o imply unlimited, I don't think the same thing applies to these params). Not a huge issue, but simpler is always better, even if its marginal. /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h <https://reviews.apache.org/r/18423/#comment65886> I'm not sure I see a need for these to be Optionals. A value of 0 is not a valid setting in either case. Why not just use that as indicating whether a value is requested or not? /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h <https://reviews.apache.org/r/18423/#comment65887> I think the same applies here. Setting the value to 0 would make no sense, so it seems a reasonable value to use to imply 'not set'. - Gordon Sim On Feb. 24, 2014, 2:11 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18423/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2014, 2:11 p.m.) > > > Review request for qpid, Chug Rolke and Gordon Sim. > > > Bugs: QPID-5561 and QPID-5565 > https://issues.apache.org/jira/browse/QPID-5561 > https://issues.apache.org/jira/browse/QPID-5565 > > > Repository: qpid > > > Description > ------- > > This patch fixes both: > > QPID-5561: C++ Broker queue creation ACL checks against wrong limit values > QPID-5565: [C++ broker] ACL queue create rules to take zero as unlimited value > > The patch: > - passes to ACL persistency-related / paging queue options only if the queue > is marked as durable / with paging > - fills proper default values > > > Potential issues: > - I had to copy constant value "static const u_int32_t defJrnlFileSizePgs = > 24" and also "defNumJrnlFiles = 8" as there is no simple way (known to me) to > test if legacy store module is loaded > - anyway as legacy store is not supposed to be changed (due to > linearstore), I see this a acceptable > - ACL line like: > > acl allow all create queue filemaxcountupperlimit=32 > > is _not_ matched for a transient queue. That differs from current behaviour, > anyway I see it as an improvement rather (as is, the above ACL is meaningless > for transient queues that dont have filecount limit applicable) > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1571198 > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.h 1571198 > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1571198 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1571198 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1571198 > /trunk/qpid/cpp/src/tests/acl.py 1571198 > > Diff: https://reviews.apache.org/r/18423/diff/ > > > Testing > ------- > > - different variants of use cases (esp. from the JIRAs) passed > - automated tests passed > > > Thanks, > > Pavel Moravec > >
