> 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. > > Andrew Stitcher wrote: > I've done this - added a pf_ prefix to the pagefile name and made the > directoryt a DataDir now to get the locking behaviour. > > Gordon Sim wrote: > Your commit breaks the tests (lots of 'Can't create directory: /pq' > errors) and the windows build (windows/MemoryMappedFile.cpp not modified for > your signature changes). Also, if a paged queue is requesed with > --no-data-dir it fails (as expected), but leaves the management object (so it > shows up in qpid-config queues). That is certainly a deficiency of the queue > lifecycle management, but moving the test into the start of > QueueFactory::create() would avoid it (and be nicer in my view anyway).
Oops, I've now fixed the breakage. Is there a specific reason why failing to create the queue because there is no data directory should be handled differently from a management perspective than failing to create it because there is no support for memory mapped files? I note that in that case no exception is raised but presumably the queue still ends up existing as far as management is concerned. It would be in keeping with the rest of QueueFactory::create() to add to the chain of if .. else if .. and just log a warning. But I don't think that would solve your underlying issue. It seems that that queue creation can (in general fail and) throw an exception, so as you imply the management lifecycle for queues probably needs fixing (although it might equally make sense to record the queue and not its creation failed and the reason). - 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 > >
