You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2014/01/29 23:09:28 UTC
Review Request 17511: Suggested fix for the paged queue file issues
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17511/
-----------------------------------------------------------
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
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17511/#review33229
-----------------------------------------------------------
Ship it!
Ship It!
- Alan Conway
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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Andrew Stitcher <as...@apache.org>.
> 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.
Oh sorry I completely misunderstood - are you are suggesting using "--no-data-dir --paging-dir"?
> 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.
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.
- 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Andrew Stitcher <as...@apache.org>.
> On Jan. 30, 2014, 9:46 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp, line 139
> > <https://reviews.apache.org/r/17511/diff/1/?file=453940#file453940line139>
> >
> > Is there a reason you don't want the path in the log messages? (To me it seems useful, and save looking up the directory in use at the time).
I removed it since the refactoring couldn't support it and I didn't see any way in which it was really useful - I couldn't see how knowing the exact path would help fix a problem with page size.
I don't very strongly about this though.
> 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).
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.
> 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.
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.
- 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Andrew Stitcher <as...@apache.org>.
> 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Andrew Stitcher <as...@apache.org>.
> 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Gordon Sim <gs...@redhat.com>.
> 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.
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).
- 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17511/#review33212
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/PagedQueue.cpp
<https://reviews.apache.org/r/17511/#comment62574>
Is there a reason you don't want the path in the log messages? (To me it seems useful, and save looking up the directory in use at the time).
/trunk/qpid/cpp/src/qpid/sys/posix/MemoryMappedFile.cpp
<https://reviews.apache.org/r/17511/#comment62575>
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).
/trunk/qpid/cpp/src/tests/run_paged_queue_tests
<https://reviews.apache.org/r/17511/#comment62576>
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.
- Gordon Sim
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
>
>
Re: Review Request 17511: Suggested fix for the paged queue file issues
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17511/#review33210
-----------------------------------------------------------
Ship it!
Patch seems fine. I tested it on several scenarios, all were fine.
The change:
- DataDir pagingDir;
+ std::string pagingDir;
also prevents the lockfile to be created.
Ship it!
- Pavel Moravec
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
>
>