> On Nov. 11, 2013, 7:31 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 220 > > <https://reviews.apache.org/r/15353/diff/2/?file=381997#file381997line220> > > > > [Just my personal preference] > > I think it would be clearer to make this: > > pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir) > > > > and make the code in the queue factory simply new PagedQueue(... > > broker->getOptions().pagingDir...) > > > > In other words the paged queue code itself doesn't know how the paging > > dir is selected it just uses the directory it is told. > > > > It also means that the broker code has the correct paging directory > > from the start if that is needed in the future. > > > > [This is just my aesthetic sense though, and the code isn't problematic > > as it is]
I don't think you can do that exactly (perhaps I am misunderstanding your intent). The 'broker->getOptions().pagingDir' won't be updated by 'pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)'. I also think its odd to have two lock file instances for the same directory and therefore the same file. Re "the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told", that is already the case (and was before this patch also. The logic for choosing a directory is currently in the QueueFactory class. Re "the broker code has the correct paging directory from the start if that is needed in the future", that is also already true in this patch if I understand you correctly. If a paging directory is specified, the broker is setup to use that on initialisation and locks it for future use at that point. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15353/#review28679 ----------------------------------------------------------- On Nov. 11, 2013, 12:30 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15353/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2013, 12:30 p.m.) > > > Review request for qpid. > > > Bugs: QPID-5316 > https://issues.apache.org/jira/browse/QPID-5316 > > > Repository: qpid > > > Description > ------- > > Adds a new option as suggested on user list, in order to control where the > paging files go speartely from the other data-dir stuff. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 > /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 > > Diff: https://reviews.apache.org/r/15353/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
