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:
signature.asc
Description: OpenPGP digital signature
