PGLog.{cc,h} review request

PGLog.{cc,h} review request

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 4947 bytes --]

Hi Sam,

I would be grateful if you could review https://github.com/dachary/ceph/commit/eeb8854a41409ef4224bf42101a19de70916573c ( compiles ok + make check runs but did not go thru teuthology or any manual tests ). It is a large patch but I expect the followups to be smaller. I did not send a pull request because it is a first attempt and you may want me to rethink what was done. It moves about 1000 lines out of the PG.{cc,h} ( i.e. 10% ). One of the benefits is that reading PGLog.h gives a synthetic view of what services the future PGLog API should provide. 

Cheers

    move log, ondisklog, missing from PG to PGLog
    
    PG::log, PG::ondisklog, PG::missing are moved from PG to a new PGLog
    class and are made protected data members. It is a preliminary step
    before writing unit tests to cover the methods that have side effects
    on these data members and define a clean PGLog API. It improves
    encapsulation and does not change any of the logic already in
    place.
    
    Possible issues :
    
    * an additional reference (PG->PGLog->IndexedLog instead of
      PG->IndexedLog for instance) is introduced : is it optimized ?
    
    * pg_log_t::splice converts its const_iterator into an iterator using
      std::advance which goes thru each element of the logs up to the
      position of the const_iterator. If the log is long and the call is
      too frequent, there will be a noticeable performance penalty. Is it
      called frequently enough to be a problem ?
    
    * rewriting log.log into pg_log.get_log().log affects the readability
      but should be optimized and have no impact on performances
    
    The guidelines followed for this patch are:
    
    * const access to the data members are preserved, no attempt is made
      to define accessors
    
    * all non const methods are in PGLog, no access to non const methods of
      PGLog::log, PGLog::logondisk and PGLog::missing are provided
    
    * when methods are moved from PG to PGLog the change to their
      implementation is restricted to the minimum.
    
    * the PG::OndiskLog and PG::IndexedLog sub classes are moved
      to PGLog sub classes unmodified and remain public
    
    A const version of the pg_log_t::find_entry method was added.
    
    A const accessor is provided for PGLog::get_log, PGLog::get_missing,
    PGLog::get_ondisklog but no non-const accessor.
    
    Arguments are added to most of the methods moved from PG to PGLog so
    that they can get access to PG data members such as info or log_oid.
    A bound method pointer to PG::remove_snap_mapped_object is provided to
    the merge_log, rewind_divergent_log and merge_old_entry. These additions
    are not repeated in the method notes below.
    
    The PGLog method are sorted according to the data member they modify.
    
    //////////////////// missing ////////////////////
    
    * The pg_missing_t::{got,have,need,add,rm} methods are wrapped as
      PGLog::missing_{got,have,need,add,rm}
    
    //////////////////// log ////////////////////
    
    * PGLog::get_tail, PGLog::get_head getters are created
    
    * PGLog::set_tail, PGLog::set_head, PGLog::set_last_requested setters are created
    
    * PGLog::index, PGLog::unindex, PGLog::add wrappers, PGLog::reset_recovery_pointers are created
    
    * PGLog::splice is a wrapper for pg_log_t::splice that converts its
      const_iterator into an iterator
    
    * PGLog::clear_info_log replaces PG::clear_info_log
    
    * PGLog::trim replaces PG::trim
    
    //////////////////// log & missing ////////////////////
    
    * PGLog::claim_log is created with code extracted from
      PG::RecoveryState::Stray::react.
    
    * PGLog::split_into is created with code extracted from
      PG::split_into.
    
    * PGLog::recover_got is created with code extracted from
      ReplicatedPG::recover_got.
    
    * PGLog::activate_not_complete is created with code extracted
      from PG::active
    
    * PGLog:proc_replica_log is created with code extracted from
      PG::proc_replica_log
    
    * PGLog:write_log is created with code extracted from
      PG::write_log
    
    * PGLog::merge_old_entry replaces PG::merge_old_entry
    
    * PGLog::rewind_divergent_log replaces PG::rewind_divergent_log
    
    * PGLog::merge_log replaces PG::merge_log
    
    * PGLog:write_log is created with code extracted from PG::write_log. A
      non-static version is created for convenience but is a simple
      wrapper.
    
    * PGLog:read_log replaces PG::read_log. A non-static version is
      created for convenience but is a simple wrapper.
    
    * PGLog:read_log_old replaces PG::read_log_old.
   


-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: PGLog.{cc,h} review request

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

Hi Sam,

The patch was amended with your recommendations and rebased against today's master. I submitted a pull request at https://github.com/ceph/ceph/pull/308

For the record, the previous patch, including your comments is located at https://github.com/dachary/ceph/commit/7267244bf0775442d445407e55b7fbb9935a404a

Cheers

On 05/19/2013 10:33 AM, Loic Dachary wrote:
> Hi Sam,
> 
> I would be grateful if you could review https://github.com/dachary/ceph/commit/eeb8854a41409ef4224bf42101a19de70916573c ( compiles ok + make check runs but did not go thru teuthology or any manual tests ). It is a large patch but I expect the followups to be smaller. I did not send a pull request because it is a first attempt and you may want me to rethink what was done. It moves about 1000 lines out of the PG.{cc,h} ( i.e. 10% ). One of the benefits is that reading PGLog.h gives a synthetic view of what services the future PGLog API should provide. 
> 
> Cheers
> 
>     move log, ondisklog, missing from PG to PGLog
>     
>     PG::log, PG::ondisklog, PG::missing are moved from PG to a new PGLog
>     class and are made protected data members. It is a preliminary step
>     before writing unit tests to cover the methods that have side effects
>     on these data members and define a clean PGLog API. It improves
>     encapsulation and does not change any of the logic already in
>     place.
>     
>     Possible issues :
>     
>     * an additional reference (PG->PGLog->IndexedLog instead of
>       PG->IndexedLog for instance) is introduced : is it optimized ?
>     
>     * pg_log_t::splice converts its const_iterator into an iterator using
>       std::advance which goes thru each element of the logs up to the
>       position of the const_iterator. If the log is long and the call is
>       too frequent, there will be a noticeable performance penalty. Is it
>       called frequently enough to be a problem ?
>     
>     * rewriting log.log into pg_log.get_log().log affects the readability
>       but should be optimized and have no impact on performances
>     
>     The guidelines followed for this patch are:
>     
>     * const access to the data members are preserved, no attempt is made
>       to define accessors
>     
>     * all non const methods are in PGLog, no access to non const methods of
>       PGLog::log, PGLog::logondisk and PGLog::missing are provided
>     
>     * when methods are moved from PG to PGLog the change to their
>       implementation is restricted to the minimum.
>     
>     * the PG::OndiskLog and PG::IndexedLog sub classes are moved
>       to PGLog sub classes unmodified and remain public
>     
>     A const version of the pg_log_t::find_entry method was added.
>     
>     A const accessor is provided for PGLog::get_log, PGLog::get_missing,
>     PGLog::get_ondisklog but no non-const accessor.
>     
>     Arguments are added to most of the methods moved from PG to PGLog so
>     that they can get access to PG data members such as info or log_oid.
>     A bound method pointer to PG::remove_snap_mapped_object is provided to
>     the merge_log, rewind_divergent_log and merge_old_entry. These additions
>     are not repeated in the method notes below.
>     
>     The PGLog method are sorted according to the data member they modify.
>     
>     //////////////////// missing ////////////////////
>     
>     * The pg_missing_t::{got,have,need,add,rm} methods are wrapped as
>       PGLog::missing_{got,have,need,add,rm}
>     
>     //////////////////// log ////////////////////
>     
>     * PGLog::get_tail, PGLog::get_head getters are created
>     
>     * PGLog::set_tail, PGLog::set_head, PGLog::set_last_requested setters are created
>     
>     * PGLog::index, PGLog::unindex, PGLog::add wrappers, PGLog::reset_recovery_pointers are created
>     
>     * PGLog::splice is a wrapper for pg_log_t::splice that converts its
>       const_iterator into an iterator
>     
>     * PGLog::clear_info_log replaces PG::clear_info_log
>     
>     * PGLog::trim replaces PG::trim
>     
>     //////////////////// log & missing ////////////////////
>     
>     * PGLog::claim_log is created with code extracted from
>       PG::RecoveryState::Stray::react.
>     
>     * PGLog::split_into is created with code extracted from
>       PG::split_into.
>     
>     * PGLog::recover_got is created with code extracted from
>       ReplicatedPG::recover_got.
>     
>     * PGLog::activate_not_complete is created with code extracted
>       from PG::active
>     
>     * PGLog:proc_replica_log is created with code extracted from
>       PG::proc_replica_log
>     
>     * PGLog:write_log is created with code extracted from
>       PG::write_log
>     
>     * PGLog::merge_old_entry replaces PG::merge_old_entry
>     
>     * PGLog::rewind_divergent_log replaces PG::rewind_divergent_log
>     
>     * PGLog::merge_log replaces PG::merge_log
>     
>     * PGLog:write_log is created with code extracted from PG::write_log. A
>       non-static version is created for convenience but is a simple
>       wrapper.
>     
>     * PGLog:read_log replaces PG::read_log. A non-static version is
>       created for convenience but is a simple wrapper.
>     
>     * PGLog:read_log_old replaces PG::read_log_old.
>    
> 
> 

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: PGLog.{cc,h} review request

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 5930 bytes --]

Hi Sam,

While expecting your review, I kept reading the code to better understand it. Since the next step involves modifying / defining the PGLog API, that will help me get it right ;-)

Cheers

On 05/22/2013 03:37 PM, Loic Dachary wrote:
> Hi Sam,
> 
> The patch was amended with your recommendations and rebased against today's master. I submitted a pull request at https://github.com/ceph/ceph/pull/308
> 
> For the record, the previous patch, including your comments is located at https://github.com/dachary/ceph/commit/7267244bf0775442d445407e55b7fbb9935a404a
> 
> Cheers
> 
> On 05/19/2013 10:33 AM, Loic Dachary wrote:
>> Hi Sam,
>>
>> I would be grateful if you could review https://github.com/dachary/ceph/commit/eeb8854a41409ef4224bf42101a19de70916573c ( compiles ok + make check runs but did not go thru teuthology or any manual tests ). It is a large patch but I expect the followups to be smaller. I did not send a pull request because it is a first attempt and you may want me to rethink what was done. It moves about 1000 lines out of the PG.{cc,h} ( i.e. 10% ). One of the benefits is that reading PGLog.h gives a synthetic view of what services the future PGLog API should provide. 
>>
>> Cheers
>>
>>     move log, ondisklog, missing from PG to PGLog
>>     
>>     PG::log, PG::ondisklog, PG::missing are moved from PG to a new PGLog
>>     class and are made protected data members. It is a preliminary step
>>     before writing unit tests to cover the methods that have side effects
>>     on these data members and define a clean PGLog API. It improves
>>     encapsulation and does not change any of the logic already in
>>     place.
>>     
>>     Possible issues :
>>     
>>     * an additional reference (PG->PGLog->IndexedLog instead of
>>       PG->IndexedLog for instance) is introduced : is it optimized ?
>>     
>>     * pg_log_t::splice converts its const_iterator into an iterator using
>>       std::advance which goes thru each element of the logs up to the
>>       position of the const_iterator. If the log is long and the call is
>>       too frequent, there will be a noticeable performance penalty. Is it
>>       called frequently enough to be a problem ?
>>     
>>     * rewriting log.log into pg_log.get_log().log affects the readability
>>       but should be optimized and have no impact on performances
>>     
>>     The guidelines followed for this patch are:
>>     
>>     * const access to the data members are preserved, no attempt is made
>>       to define accessors
>>     
>>     * all non const methods are in PGLog, no access to non const methods of
>>       PGLog::log, PGLog::logondisk and PGLog::missing are provided
>>     
>>     * when methods are moved from PG to PGLog the change to their
>>       implementation is restricted to the minimum.
>>     
>>     * the PG::OndiskLog and PG::IndexedLog sub classes are moved
>>       to PGLog sub classes unmodified and remain public
>>     
>>     A const version of the pg_log_t::find_entry method was added.
>>     
>>     A const accessor is provided for PGLog::get_log, PGLog::get_missing,
>>     PGLog::get_ondisklog but no non-const accessor.
>>     
>>     Arguments are added to most of the methods moved from PG to PGLog so
>>     that they can get access to PG data members such as info or log_oid.
>>     A bound method pointer to PG::remove_snap_mapped_object is provided to
>>     the merge_log, rewind_divergent_log and merge_old_entry. These additions
>>     are not repeated in the method notes below.
>>     
>>     The PGLog method are sorted according to the data member they modify.
>>     
>>     //////////////////// missing ////////////////////
>>     
>>     * The pg_missing_t::{got,have,need,add,rm} methods are wrapped as
>>       PGLog::missing_{got,have,need,add,rm}
>>     
>>     //////////////////// log ////////////////////
>>     
>>     * PGLog::get_tail, PGLog::get_head getters are created
>>     
>>     * PGLog::set_tail, PGLog::set_head, PGLog::set_last_requested setters are created
>>     
>>     * PGLog::index, PGLog::unindex, PGLog::add wrappers, PGLog::reset_recovery_pointers are created
>>     
>>     * PGLog::splice is a wrapper for pg_log_t::splice that converts its
>>       const_iterator into an iterator
>>     
>>     * PGLog::clear_info_log replaces PG::clear_info_log
>>     
>>     * PGLog::trim replaces PG::trim
>>     
>>     //////////////////// log & missing ////////////////////
>>     
>>     * PGLog::claim_log is created with code extracted from
>>       PG::RecoveryState::Stray::react.
>>     
>>     * PGLog::split_into is created with code extracted from
>>       PG::split_into.
>>     
>>     * PGLog::recover_got is created with code extracted from
>>       ReplicatedPG::recover_got.
>>     
>>     * PGLog::activate_not_complete is created with code extracted
>>       from PG::active
>>     
>>     * PGLog:proc_replica_log is created with code extracted from
>>       PG::proc_replica_log
>>     
>>     * PGLog:write_log is created with code extracted from
>>       PG::write_log
>>     
>>     * PGLog::merge_old_entry replaces PG::merge_old_entry
>>     
>>     * PGLog::rewind_divergent_log replaces PG::rewind_divergent_log
>>     
>>     * PGLog::merge_log replaces PG::merge_log
>>     
>>     * PGLog:write_log is created with code extracted from PG::write_log. A
>>       non-static version is created for convenience but is a simple
>>       wrapper.
>>     
>>     * PGLog:read_log replaces PG::read_log. A non-static version is
>>       created for convenience but is a simple wrapper.
>>     
>>     * PGLog:read_log_old replaces PG::read_log_old.
>>    
>>
>>
> 

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]