> 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. > > Gordon Sim wrote: > 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.
I've done this - added a pf_ prefix to the pagefile name and made the directoryt a DataDir now to get the locking behaviour. - Andrew ----------------------------------------------------------- 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 > >
