Opened 7 years ago

Closed 7 years ago

#2208 closed task (fixed)

Revise InMemoryClient and ConfigurableClientList::configure() using ZoneTableSegment

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121106
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: 5 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by jinmei)

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

A subtask of #2201, depend on #2206.

In this task, we update the following part of this method:

                const shared_ptr<InMemoryClient>
                    cache(new_data_sources.back().cache_);
                ...
                for (vector<string>::const_iterator it(zones_origins.begin());
                     it != zones_origins.end(); ++it) {
                    ...
                }

using new concepts introduced in this feature work. For simplicity,
we assume the use of MemorySegmentLocal within this class for the
moment, but still use the base class wherever possible.

First, the revised InMemoryClient will have the following private
members:

class InMemoryClient : public DataSourceClient {
    // ...
private:
    boost::shared_ptr<ZoneTableSegment> table_segment_; // or scoped_ptr
};

As shown in http://bind10.isc.org/wiki/ScalableZoneLoadDesign#a4.UsingSegmentsfromIn-MemoryDataSourceClient
but for simplicity we don't use ZoneSegments. For the moment,
everything will be stored in ZoneTableSegment.

Then, the above code to be revised would look like this:

    ztable_segment = shared_ptr<ZoneTableSegment>(
        ZoneTableSegment::create(config));
    for (vector<string>::const_iterator it(zones_origins.begin());
         it != zones_origins.end(); ++it) {
        zone_data = ZoneData::create(ztable_segment->getMemorySegment(), *it);
        ztable_segment->getHeader()->table->insert(zone_data);
    }

    // ideally we should set it in the InMemory constructor
    const shared_ptr<InMemoryClient>
        cache(new_data_sources.back().cache_);
    cache->setZoneTable(ztable_segment);

Some other parts of InMemoryClient will also have to be adjusted due
to the introduction of table_segment_.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jelte

  • Milestone set to Sprint-20121023

comment:4 Changed 7 years ago by muks

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

Picking.

comment:5 Changed 7 years ago by muks

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

Up for review.

comment:6 follow-up: Changed 7 years ago by jelte

  • Owner changed from UnAssigned to muks

Could you please provide a few hints for the reviewer next time? For instance, why is RRClass now a member of ZoneTable? instead of passing it from the client like before?

Isn't that hardcoded RRClass::IN() going to present a problem (given that the 'static' datasource also uses in-memory)?

I think it would be a bit more clean to consistently call arguments and members ztable_segment (a few of them are now named the more general 'segment' in client_list)

on a smaller note, you have a number of cases where you use *somesharedptr.get() which can be written with just *somesharedptr (at least in client_list.cc:96 and client_list_unittest.cc:260), but maybe more

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

Hello

I'd like to ask for a little thing. If #2207 is merged before this one, I added
a temporary method for ZoneTableHeader (setTable). It is used only in tests
and it seems your branch does it the correct way (eg. passing one on
construction). Could you remove the method and modify the tests
(zone_writer_unittest.cc, the test suite constructor) when merging?

Thank you

comment:8 in reply to: ↑ 7 Changed 7 years ago by muks

Replying to vorner:

Hello

I'd like to ask for a little thing. If #2207 is merged before this one, I added
a temporary method for ZoneTableHeader (setTable). It is used only in tests
and it seems your branch does it the correct way (eg. passing one on
construction). Could you remove the method and modify the tests
(zone_writer_unittest.cc, the test suite constructor) when merging?

Thank you

Will do a merge from master after reviewing existing branch is done.

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

  • Owner changed from muks to jelte

Hi Jelte

Replying to jelte:

Could you please provide a few hints for the reviewer next time? For instance, why is RRClass now a member of ZoneTable? instead of passing it from the client like before?

Ouch, that commit log message is inadequate. Basically, RRClass has to be a property of ZoneTable, which is used by it during its destruction. We do the pass-by-caller trick to avoid bloating some objects, but it doesn't apply to ZoneTable as there aren't many of them. In this branch, this change was introduced as otherwise, other classes such as ZoneTableSegment would have to manage the RRClass (and such code doesn't belong there).

Isn't that hardcoded RRClass::IN() going to present a problem (given that the 'static' datasource also uses in-memory)?

I am assuming you mean the hardcoding inside ZoneTableSegment's factory method. We have to decide upon config syntax for it. The factory would construct the appropriate ZoneTableSegment based on the passed memory model, RRClass, etc. in config.

Also, currently the static datasrc doesn't use the new in-memory code. But we have to address this issue when we decide upon config.

I think it would be a bit more clean to consistently call arguments and members ztable_segment (a few of them are now named the more general 'segment' in client_list)

Done. :)

on a smaller note, you have a number of cases where you use *somesharedptr.get() which can be written with just *somesharedptr (at least in client_list.cc:96 and client_list_unittest.cc:260), but maybe more

Done. :)

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

  • Owner changed from jelte to muks

Replying to muks:

Isn't that hardcoded RRClass::IN() going to present a problem (given that the 'static' datasource also uses in-memory)?

I am assuming you mean the hardcoding inside ZoneTableSegment's factory method. We have to decide upon config syntax for it. The factory would construct the appropriate ZoneTableSegment based on the passed memory model, RRClass, etc. in config.

Also, currently the static datasrc doesn't use the new in-memory code. But we have to address this issue when we decide upon config.

Right, but I'm hesitant to add more to do before we can switch that one :)

But isn't the class already in the configuration? (though from the looks of it at a 'higher' level than what is currently passed to the factory). I don't think we're gonna put class in there twice, and I don't think there are any plans to move it away.

The clientlist already knows it afaict, so perhaps it should just be passed to the factory function there.

For the rest it looks fine

comment:11 in reply to: ↑ 10 Changed 7 years ago by muks

  • Owner changed from muks to jelte

Replying to jelte:

Replying to muks:

Isn't that hardcoded RRClass::IN() going to present a problem (given that the 'static' datasource also uses in-memory)?

I am assuming you mean the hardcoding inside ZoneTableSegment's factory method. We have to decide upon config syntax for it. The factory would construct the appropriate ZoneTableSegment based on the passed memory model, RRClass, etc. in config.

Also, currently the static datasrc doesn't use the new in-memory code. But we have to address this issue when we decide upon config.

Right, but I'm hesitant to add more to do before we can switch that one :)

But isn't the class already in the configuration? (though from the looks of it at a 'higher' level than what is currently passed to the factory). I don't think we're gonna put class in there twice, and I don't think there are any plans to move it away.

The clientlist already knows it afaict, so perhaps it should just be passed to the factory function there.

Done now. :)

comment:12 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

awesome :)

One final small thing, It looks like the rrclass arguments can be const references. But with that it can be merged

comment:13 in reply to: ↑ 12 Changed 7 years ago by muks

Replying to jelte:

awesome :)

One final small thing, It looks like the rrclass arguments can be const references. But with that it can be merged

I wrote part of such a change before giving the bug back to you :) .. and then reset my tree because there is no consistency in the code about it. RRClass are references in some places, and not in others. I just followed the nearest convention. See createSQLite3Client() on to DatabaseClient(), stuff in isc::datasrc::memory such as RdataSet, ZoneData, InMemoryClient, etc.

Now the code which is modified by this branch (ZoneTable and ZoneTableSegment::create()) has been updated to use const references instead. ZoneTable keeps a const copy.

comment:14 Changed 7 years ago by muks

  • Owner changed from muks to jelte

comment:15 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

ok, looks good, go ahead and merge

comment:16 in reply to: ↑ 15 Changed 7 years ago by jinmei

Replying to jelte:

ok, looks good, go ahead and merge

Actually, please just close the ticket.

#2208 merged in #2209 with fixing conflicts. We confirmed the merge
and are going to merge #2209 soon. Simply merging the resulting #2209
would be better at this stage.

comment:17 Changed 7 years ago by muks

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

OK closing bug in this case.

Note: See TracTickets for help on using tickets.