> 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. > > Gordon Sim wrote: > 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. > > Andrew Stitcher wrote: > One of the symptoms of the original bug was that you can't call your > queue "lock" because there is already a file called lock. > > My thought was that if you used the default location (relative to data > dir) then you are already locked and if you specified a separate paging > directory then that the admin needs to ensure that it is used exclusively - I > can see that latter position isn't very friendly. > > Perhaps we ought to use the broker id in the the directory path to avoid > this scenario, so we have <paging-dir>/<broker-id>/<paging-filename>? > > To amplify creating the directory there - open takes both a name and a > directory - it creates a file based in the name I don't see any substantial > difference in having it create the directory if necessary as well.
I think avoiding a lock file so that you don't have a filename collision is the wrong way to fix that. Adding a simple pg prefix to the queue name (or any other scheme to mangle the name as you suggest) would avoid that. > 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. > > Gordon Sim wrote: > 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. > > Andrew Stitcher wrote: > Oh sorry I completely misunderstood - are you are suggesting using > "--no-data-dir --paging-dir"? Yes, "--no-data-dir --paging-dir" would work fine provided the paging dir was unique to this test run (also using --data-dir <unique path> would work). In the ideal word I guess we would use both to exercse the two modes of configuration, but my concern was not about coverage but about avoiding failures when ci kicks of two runs on the same box (or even when I do that myself!) - 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 > >
