> On Jan. 30, 2014, 9:46 a.m., Gordon Sim wrote: > > /trunk/qpid/cpp/src/qpid/sys/posix/MemoryMappedFile.cpp, line 68 > > <https://reviews.apache.org/r/17511/diff/1/?file=453944#file453944line68> > > > > Personally I think having the directory creation outside this class > > makes more sense. Having a directory created if specified as an option > > seems reasonable. If there was ever any other use of this though, it may > > well be an error not to have the directory there. > > > > What is the rationale for this change? (As Pavel points out it also > > means there is no lock file created, which I think is important). > > Andrew Stitcher wrote: > Not having the lock file was a requirement! As it takes a queue name. > Also in the default case the data dir is already locked so there doesn't need > to be a second lock. > > I can see that a separate paging directory might need protection from > another broker process though.
I don't understand the first two sentences. Having the lock file for the separate paging directory if used is important. If the data dir is used then obviously it already has one. > On Jan. 30, 2014, 9:46 a.m., Gordon Sim wrote: > > /trunk/qpid/cpp/src/tests/run_paged_queue_tests, line 29 > > <https://reviews.apache.org/r/17511/diff/1/?file=453945#file453945line29> > > > > Removing the --no-data-dir option while not providing a data-dir means > > that the default will be used which will fail if any other broker is > > running using the default on the machine. I.e. it will likely cause CI > > issues. > > Andrew Stitcher wrote: > I don't understand this issue. What do you mean by "default"? If you mean > the pidfile dir then I think this is a terrible concept and wrongly conceived > for a system daemon which should have a current directory of '/' to avoid > locking a system mount. > > In any case why would I make /run/pids (or whatever it is called) the > place to put paging files? > > So in my view the only meaningful default dir should be the data dir and > if it is not specified then there is no default. There is a default data dir. Using the --no-data-dir option prevents that being used (or created if it doesn't exist). Removing that option without further changes will break concurrent runs of the test. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17511/#review33212 ----------------------------------------------------------- On Jan. 29, 2014, 10:09 p.m., Andrew Stitcher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17511/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2014, 10:09 p.m.) > > > Review request for qpid, Alan Conway, Gordon Sim, and Pavel Moravec. > > > Bugs: QPID-5485 and QPID-5486 > https://issues.apache.org/jira/browse/QPID-5485 > https://issues.apache.org/jira/browse/QPID-5486 > > > Repository: qpid > > > Description > ------- > > 1. Use a directory solely for paged queue files, so there can be no file name > clashes. > 2. Don't require exclusive file access, and truncate any previous file. > 3. by default use <datadir>/pq if paging-dir not specified otherwise use > paging-dir. > 4. If neither specified then error on page queue creation (this was broken > before and would create files in the current directory which in the tests > would succeed accidentally because it would be ~/.qpidd because this is both > the default data dir and pidfile dir) > 5. Tiny refactoring in who holds the final page file path. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Broker.h 1562539 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1562539 > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.h 1562539 > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp 1562539 > /trunk/qpid/cpp/src/qpid/broker/posix/BrokerDefaults.cpp 1562539 > /trunk/qpid/cpp/src/qpid/broker/windows/BrokerDefaults.cpp 1562539 > /trunk/qpid/cpp/src/qpid/sys/MemoryMappedFile.h 1562539 > /trunk/qpid/cpp/src/qpid/sys/posix/MemoryMappedFile.cpp 1562539 > /trunk/qpid/cpp/src/tests/run_paged_queue_tests 1562539 > > Diff: https://reviews.apache.org/r/17511/diff/ > > > Testing > ------- > > ctest -R page (both as is now and with the qpidd start line massaged with > --no-data-dir so it should fail) > > > Thanks, > > Andrew Stitcher > >
