You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2013/11/08 18:44:02 UTC

Review Request 15353: Add --paging-dir broker option

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/
-----------------------------------------------------------

Review request for qpid.


Bugs: QPID-5316
    https://issues.apache.org/jira/browse/QPID-5316


Repository: qpid


Description
-------

Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.


Diffs
-----

  /trunk/qpid/cpp/src/qpid/DataDir.h 1540138 
  /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540138 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540138 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540138 
  /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540138 

Diff: https://reviews.apache.org/r/15353/diff/


Testing
-------


Thanks,

Gordon Sim

Re: Review Request 15353: Add --paging-dir broker option

Posted by Andrew Stitcher <as...@apache.org>.

> On Nov. 11, 2013, 7:31 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 220
> > <https://reviews.apache.org/r/15353/diff/2/?file=381997#file381997line220>
> >
> >     [Just my personal preference]
> >     I think it would be clearer to make this:
> >     pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)
> >     
> >     and make the code in the queue factory simply new PagedQueue(... broker->getOptions().pagingDir...)
> >     
> >     In other words the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told.
> >     
> >     It also means that the broker code has the correct paging directory from the start if that is needed in the future.
> >     
> >     [This is just my aesthetic sense though, and the code isn't problematic as it is]
> 
> Gordon Sim wrote:
>     I don't think you can do that exactly (perhaps I am misunderstanding your intent). The 'broker->getOptions().pagingDir' won't be updated by 'pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)'. I also think its odd to have two lock file instances for the same directory and therefore the same file.
>     
>     Re "the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told", that is already the case (and was before this patch also. The logic for choosing a directory is currently in the QueueFactory class.
>     
>     Re "the broker code has the correct paging directory from the start if that is needed in the future", that is also already true in this patch if I understand you correctly. If a paging directory is specified, the broker is setup to use that on initialisation and locks it for future use at that point.

The point about using the same directory for 2 different DataDir object is a good one, and in fact I think my idea won't work because the second DataDir won't be able to take the lock (which is after all meant to ensure exclusive access).

What I meant about the paging code not knowing whether it was using the dataDir or the pagingDir was just to minimise it's configuration "bandwidth" - It would be better to just pass it a single DataDir object to use rather than have it select from 2 depending on how they are configured. I'm saying I think it should be the broker object responsibility to figure out the directory and not the pagedQueue itself.

In any case what is in this proposal is fine, my thoughts really just expose some limits on what the underlying abstractions are capable of.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28679
-----------------------------------------------------------


On Nov. 11, 2013, 12:30 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 12:30 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Gordon Sim <gs...@redhat.com>.

> On Nov. 11, 2013, 7:31 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp, line 220
> > <https://reviews.apache.org/r/15353/diff/2/?file=381997#file381997line220>
> >
> >     [Just my personal preference]
> >     I think it would be clearer to make this:
> >     pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)
> >     
> >     and make the code in the queue factory simply new PagedQueue(... broker->getOptions().pagingDir...)
> >     
> >     In other words the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told.
> >     
> >     It also means that the broker code has the correct paging directory from the start if that is needed in the future.
> >     
> >     [This is just my aesthetic sense though, and the code isn't problematic as it is]

I don't think you can do that exactly (perhaps I am misunderstanding your intent). The 'broker->getOptions().pagingDir' won't be updated by 'pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)'. I also think its odd to have two lock file instances for the same directory and therefore the same file.

Re "the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told", that is already the case (and was before this patch also. The logic for choosing a directory is currently in the QueueFactory class.

Re "the broker code has the correct paging directory from the start if that is needed in the future", that is also already true in this patch if I understand you correctly. If a paging directory is specified, the broker is setup to use that on initialisation and locks it for future use at that point.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28679
-----------------------------------------------------------


On Nov. 11, 2013, 12:30 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 12:30 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28679
-----------------------------------------------------------



/trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/15353/#comment55614>

    [Just my personal preference]
    I think it would be clearer to make this:
    pagingDir(conf.pagingDir.empty() ? dataDir : conf.pagingDir)
    
    and make the code in the queue factory simply new PagedQueue(... broker->getOptions().pagingDir...)
    
    In other words the paged queue code itself doesn't know how the paging dir is selected it just uses the directory it is told.
    
    It also means that the broker code has the correct paging directory from the start if that is needed in the future.
    
    [This is just my aesthetic sense though, and the code isn't problematic as it is]


- Andrew Stitcher


On Nov. 11, 2013, 12:30 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 12:30 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28699
-----------------------------------------------------------

Ship it!


- Andrew Stitcher


On Nov. 11, 2013, 12:30 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 12:30 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28738
-----------------------------------------------------------

Ship it!


Ship It!

- Andrew Stitcher


On Nov. 12, 2013, 9:42 a.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2013, 9:42 a.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/
-----------------------------------------------------------

(Updated Nov. 12, 2013, 9:42 a.m.)


Review request for qpid.


Changes
-------

How about this... this update moves the logic that chooses the path out of the QueueFactory and into the Broker. Is this closer to what you had in mind?


Bugs: QPID-5316
    https://issues.apache.org/jira/browse/QPID-5316


Repository: qpid


Description
-------

Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.


Diffs (updated)
-----

  /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
  /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 

Diff: https://reviews.apache.org/r/15353/diff/


Testing
-------


Thanks,

Gordon Sim

Re: Review Request 15353: Add --paging-dir broker option

Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/
-----------------------------------------------------------

(Updated Nov. 11, 2013, 12:30 p.m.)


Review request for qpid.


Changes
-------

Make pagingDir another DataDir instance. Pull specific log message from DataDir into Broker.


Bugs: QPID-5316
    https://issues.apache.org/jira/browse/QPID-5316


Repository: qpid


Description
-------

Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.


Diffs (updated)
-----

  /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540676 
  /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540676 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540676 
  /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540676 

Diff: https://reviews.apache.org/r/15353/diff/


Testing
-------


Thanks,

Gordon Sim

Re: Review Request 15353: Add --paging-dir broker option

Posted by Jakub Scholz <ww...@scholzj.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28601
-----------------------------------------------------------


I can't really comment on the code. But as far as the functionality goes it works exactly as I expected.

- Jakub Scholz


On Nov. 8, 2013, 5:44 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2013, 5:44 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.h 1540138 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540138 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 15353: Add --paging-dir broker option

Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15353/#review28552
-----------------------------------------------------------



/trunk/qpid/cpp/src/qpid/DataDir.cpp
<https://reviews.apache.org/r/15353/#comment55351>

    I'm not sure I see the need for changing the DataDir class: Can't you just make the pagingDir another instance of a DataDir?



/trunk/qpid/cpp/src/qpid/DataDir.cpp
<https://reviews.apache.org/r/15353/#comment55350>

    I think moving the code to actually create the data directory into the "create lockfile" code isn't the correct division of functionality.


- Andrew Stitcher


On Nov. 8, 2013, 5:44 p.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15353/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2013, 5:44 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: QPID-5316
>     https://issues.apache.org/jira/browse/QPID-5316
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Adds a new option as suggested on user list, in order to control where the paging files go speartely from the other data-dir stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/DataDir.h 1540138 
>   /trunk/qpid/cpp/src/qpid/DataDir.cpp 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1540138 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1540138 
> 
> Diff: https://reviews.apache.org/r/15353/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>