> 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). > > Andrew Stitcher wrote: > 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).
If there is no support for memory mapped files then a warning is logged and you get a normal queue. The problem is that at present exceptions in QueueFactory::create() after the queue is created, are not handled and the Queue instance is not marked destroyed(), and so the management object is not destroyed. By all means fix that. Personally I think checking the directory before calling the destructor is better, and if it was to throw an exception if none is set (rather than just reverting to a regular queue), then I would do that before creating any part of the queue. - 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 > >
