Opened 7 years ago

Closed 7 years ago

#2206 closed task (fixed)

define and implement ZoneTableSegment class

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121009
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: background zone loading
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 0.75 Internal?: no

Description (last modified by jinmei)

(We probably don't have time to do this for the September release)

A subtask of #2201.

The basic concept is described in
http://bind10.isc.org/wiki/ScalableZoneLoadDesign.
We also extend the class to encapsulate zone loading logic that can
differ depending on the underlying memory segment model.

A conceptual definition of this class is as follows:

class ZoneTableSegment {
public:
    // Factory method.
    // Create a subclass depending on the memory segment model (which would
    // be config).
    // For now, we simply always create ZoneTableSegment.
    static ZoneTableSegment* create(const data::Element& config);

    virtual ZoneTableHeader* getHeader() = 0;

    // Return the underlying memory segment.
    virtual MemorySegment& getMemorySegment() = 0;

#ifdef notyet
    // Factory of zone updater context using the same memory model as the
    // zone table (and maybe internally using zone table itself).
    // config would store the zone name to be loaded or zone segment ID, etc.
    // load_action will be called from updater->load() to perform
    // caller-specific behavior (in the case of local memory segment, this
    // action will actually load the zone content to the memory)
    // install_action will be called from updater->install() to perform
    // caller-specific behavior (in the case of local memory segment, this
    // would probably be mostly NO-OP)
    virtual ZoneUpdater* getZoneUpdater(
        const data::Element& config,
        boost::function<void(memory::ZoneData*)> load_action,
        boost::function<void(ZoneSegmentID, ZoneSegment*> install_action)) = 0;
#endif
};

For now, we only define create() and getHeader().

Also define the trivial ZoneTableHeader:

struct ZoneTableHeader {
    offset_ptr<memory::ZoneTable> table;
};

And introduce the ZoneTableSegmentLocal derived class:

// ZoneTableSegment using the "local" memory segment
class ZoneTableSegmentLocal : public ZoneTableSegment {
public:
#ifdef notyet
    virtual ZoneUpdater* getZoneUpdater() {
        return (new ZoneUpdaterLocal(*this));
    }
#endif
private:
    MemorySegmentLocal mem_sgmt_;
};

Subtickets

Change History (13)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 7 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121009

comment:5 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking.

comment:6 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Up for review. This introduces the basic ZoneTableSegment and the derived ZoneTableSegmentLocal as described above (except the commented parts). Tests were written, but these just test the structural API and not anything behavioural. More of the ZoneTableSegment? implementation have to be added before it can be tested.

comment:7 follow-up: Changed 7 years ago by jinmei

zone_table_segment.h

  • brief descriptions are mostly meaningless:
    /// \brief Zone Table Header Class
    struct ZoneTableHeader {
    
    It's too obvious that this is a "Zone Table Header" class (or struct) from the type name. I'd give some more informative one-line summary of what this class is. I'd also provide some longer version of description to this particular class. Some points also apply to other classes.
  • the return value description is mostly redundant:
        /// Returns the MemorySegment used in this zone table segment.
        ///
        /// \return Returns a ZoneTableHeader for this zone table segment.
    
    I'd either provide more meaningful description or if there really isn't any just skipping the detailed version of the description.
  • ZoneTableHeader: maybe it's better to provide an accessor to table rather than having the caller be aware of offset_ptr (which is a kind of details that the caller doesn't have to care about):
    struct ZoneTableHeader {
    public:
        ZoneTable* getTable() { return (table.get()); }
        // maybe we need a const version too?
    private:
        boost::interprocess::offset_ptr<ZoneTable> table;
    };
    
  • getHeader() should never return NULL. I'd note that (or maybe we should return a reference instead of a pointer? I'm not sure which one is better).
  • I'd note that the object returned by create() is dynamically allocated and the caller is responsible for deleting. (providing a corresponding destroy() method maybe cleaner).
  • maybe a matter of taste in practice, I'd explicitly define the constructor of the base class as private to make it clear that the factory must be used to instantiate it.
  • not fully sure about this right now, but we probably want to have a const version of getHeader(). The lookup code path should only need read-only access to the table.

zone_table_segment_local

  • getHeader() and getMemorySegment(): for such very simple methods we might just define them in the header file. It's purely a taste matter though, as they are virtual methods.

zone_table_segment_unittest

  • (general) scoped_ptr would be a slightly better choice than auto_ptr for such usage.
  • config is generated in every test. You can avoid by introducing a fixture. The total amount of code length wouldn't be much different though.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to muks

comment:9 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

zone_table_segment.h

  • brief descriptions are mostly meaningless:
    /// \brief Zone Table Header Class
    struct ZoneTableHeader {
    
    It's too obvious that this is a "Zone Table Header" class (or struct) from the type name. I'd give some more informative one-line summary of what this class is. I'd also provide some longer version of description to this particular class. Some points also apply to other classes.

Added.

  • the return value description is mostly redundant:
        /// Returns the MemorySegment used in this zone table segment.
        ///
        /// \return Returns a ZoneTableHeader for this zone table segment.
    
    I'd either provide more meaningful description or if there really isn't any just skipping the detailed version of the description.

The redundant descriptions have been removed.

  • ZoneTableHeader: maybe it's better to provide an accessor to table rather than having the caller be aware of offset_ptr (which is a kind of details that the caller doesn't have to care about):
    struct ZoneTableHeader {
    public:
        ZoneTable* getTable() { return (table.get()); }
        // maybe we need a const version too?
    private:
        boost::interprocess::offset_ptr<ZoneTable> table;
    };
    

Added.

  • getHeader() should never return NULL. I'd note that (or maybe we should return a reference instead of a pointer? I'm not sure which one is better).

I think the reference is better too, if it must point to something always. Updated in code.

  • I'd note that the object returned by create() is dynamically allocated and the caller is responsible for deleting. (providing a corresponding destroy() method maybe cleaner).

Added.

  • maybe a matter of taste in practice, I'd explicitly define the constructor of the base class as private to make it clear that the factory must be used to instantiate it.

These have been made protected, and a friend as been added so that the factory method can instantiate it.

  • not fully sure about this right now, but we probably want to have a const version of getHeader(). The lookup code path should only need read-only access to the table.

Added. Though not necessary, I've made the return values const in this case too.

zone_table_segment_local

  • getHeader() and getMemorySegment(): for such very simple methods we might just define them in the header file. It's purely a taste matter though, as they are virtual methods.

I'll leave them in zone_table_segment_local.cc for now as that file is otherwise bare. I've added a comment to this file to move them once it has more method definitions.

zone_table_segment_unittest

  • (general) scoped_ptr would be a slightly better choice than auto_ptr for such usage.

This code is gone now as a fixture has been added.

  • config is generated in every test. You can avoid by introducing a fixture. The total amount of code length wouldn't be much different though.

A fixture has been added now.

comment:10 in reply to: ↑ 9 Changed 7 years ago by jinmei

Replying to muks:

I've made one small fix.

The revised version generally looks good. One minor possible concern
is that the getHeader and getHeaderConst tests are mostly duplicate.
But since the amount of code is small, it's probably okay.

So, if you want to combine these somehow, please do. Otherwise,
please feel free to merge.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:12 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 0.75

comment:13 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master in commit 41538b7ce9d6ab4131b572b79d84292884cf4a11:

* 78dfd7d [2268] Move both const and non-const checks into a single testcase
* 47c013b [2206] Make a common function out of duplicate code
* 3d68f76 [2206] editorial fix: combine two short lines
* 37fd8a7 [2206] Add comment about moving methods to the header later
* 1fb24f5 [2206] Change getHeader() to return a reference
* c4c3946 [2206] Remove redundant return descriptions
* 5c97541 [2206] Fix doc comment
* 538a160 [2206] Add const variant of getHeader()
* e451152 [2206] Fix const variant of getTable()
* a072ee8 [2206] Add const method tests
* 473e340 [2206] Update comments
* 94656c0 [2206] Make constructors protected
* a6748ee [2206] Use a fixture in tests
* 5acf0ff [2206] Add a ZoneTableSegment::destroy() method
* 1468031 [2206] Add a doc comment that getHeader() should never return NULL
* d25bb8d [2206] Add ZoneTableHeader::getTable() method
* f2b62e2 [2206] Update doc comments to remove redundant bits
* 1ab6aaf [2206] Define and implement ZoneTableSegment class

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.