Opened 8 years ago

Closed 7 years ago

#2209 closed task (complete)

define and implement ConfigurableClientList::getCacheZoneUpdater()

Reported by: jinmei Owned by: vorner
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: 12.8 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, #2207.

This is a new method to ConfigurableClientList and will eventually
replace reload(). conceptually it would look this:

shared_ptr<memory::ZoneUpdater>
ConfigurableClientList::getCacheZoneUpdater(config) {
    // identify the in-memory client
    if (type == "MasterFiles") {
        updater = mem_client->ztable_segment->getZoneUpdater(
            config,
            boost::bind(loadMemoryZoneFromFile, master_file, _1),
            boost::bind(&InMemoryClient::installZone, mem_client, _1));
        return (updater);
    } else {
        updater = mem_client->ztable_segment->getZoneUpdater(
            config,
            boost::bind(loadMemoryZoneFromDataSource, backend_client, _1),
            boost::bind(&InMemoryClient::installZone, mem_client, _1));
        return (updater);
        
    }
}

void
loadMemoryZoneFromFile(ZoneData* zone_data, string zone_file) {
    // fill in zone_data from zone_file
}

void
loadMemoryZoneFromDataSource(ZoneData* zone_data, DataSourceClient* backend) {
    // fill in zone_data using backends' iterator
}

We should already have something close to the two load_action
callbacks. We should only have to adjust the interface.

Unlike reload(), 'config' is more generic, and the content varies
depending on the underlying memory segment model. When we use the
local memory segment (which is the case for the moment) it would
simply contain the information of zone name; when we use a shared
memory segment, it will simply contain shared segment info for the new
zone segment. The getZoneUpdater() hides these details from the
ConfigurableClientList implementation.

installZone would reset the corresponding zone segment for shared
memory version. For the moment that could be an empty functor object.

Subtickets

Change History (18)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 8 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jelte

  • Milestone set to Sprint-20121023

comment:5 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

The branch is ready for review. But there's a catch.

It turned out the branch depended on #2208 (and I didn't know the ticket number
when I hit the problem). So I added a „hack“ to get some zone table segment
into the in-memory client in a92defd1deceebc13a7bcc93cee6579ed42be41e. I think
#2208 will get merged soon, so once this branch is ready for merge, I'll remove
the hack and adjust the rest. But I don't merge for now, since it would make it
harder to review.

Also, I made the reload tests templated and reused them to test the new method.
Once the reload is no longer needed, we could un-template them again. Or we can
keep reload, I turned it into a slim wrapper around the new method, so there's
not much extra code.

Also, there are two small unrelated clean ups. I could remove them if they were
problematic, but I think at least one of them is an obvious and serious bug.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

others

I made a few minor style(-ish) changes.

client_list.h

  • ReloadResult: ZONE_RELOADED is not really appropriate for the return value of getCachedZoneWriter(). We need to either rename it something more general or introduce a separate success code for getCachedZoneWriter() (note: the latter has a drawback that it will increase the risk of having a missing case in switch-case usage).
  • ZoneWriterPtr: it looks a bit awkward to be defined within ConfigurableClientList especially if it's public. I'd suggest either hiding it as private or move it to memory/zone_writer.h.

client_list.cc

  • revised reload(): I feel a bit nervous about using result.first as the return value. While the code is actually correct, we do a lot of things (actual loading job) between getting the 'result' and returning it. It also implicitly assumes if result.second is true (non NULL) the first should be ZONE_RELOADED (again, not incorrect, but makes me nervous too). I'd rewrite it a bit, for example, as follows:
        const ZoneWriterPair result(getCachedZoneWriter(name));
        if (result.first != ZONE_RELOADED) {
            return (result.first);
        }
    
        assert(result.second);
        result.second->load();
        result.second->install();
        result.second->cleanup();
    
        return (ZONE_RELOADED);
    
    as for whether we still need reload(), I don't have a strong opinion. But if we don't see specific need for it, it's probably better to clean it up (maybe later) in the sense of YAGNI. Any existing code increases maintenance costs.

memory_client.cc

  • InMemoryClient constructor: it seems to be not exception safe about zone_table_segment_. This may become a non issue if you migrate to the "formal" version of ZoneTableSegment::create, but we should check that again.

memory_client.h

  • getZoneTableSegment(): I guess we should eventually let ClientList (or at least something outside of InMemoryClient) directly manage ZoneTableSegment, at which point InMemoryClient will probably take it as a parameter to the constructor. But for now I can live with this workaround.

client_list_unittest

  • what's "plaping"? You mean "playing"?
    // plaping with that). Once we deprecate reload(), we should revert this
    
  • not matter much, and may even become a non issue depending on what to do with reload(), but I found TYPED_TEST is often heavy in building (due to the use of template). If we keep supporting both, maybe we should consider value-parameterized test (TEST_P). It's particular so in this case because the use of different types seem to be a hack just so they can be used in TYPED_TEST.
  • this comment doesn't parse:
            // Can't use ASSERT_NE here, it would wan't to return(), which
            // it can't in non-void function.
    
    "wan't" should be "want"?

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • ReloadResult: ZONE_RELOADED is not really appropriate for the return value of getCachedZoneWriter(). We need to either rename it something more general or introduce a separate success code for getCachedZoneWriter() (note: the latter has a drawback that it will increase the risk of having a missing case in switch-case usage).

Sorry, I was postponing the change during the work, until I forgot about it O:-).

  • revised reload(): I feel a bit nervous about using result.first as the return value. While the code is actually correct, we do a lot of things (actual loading job) between getting the 'result' and returning it. It also implicitly assumes if result.second is true (non NULL) the first should be ZONE_RELOADED (again, not incorrect, but makes me nervous too). I'd rewrite it a bit, for example, as follows:
        const ZoneWriterPair result(getCachedZoneWriter(name));
        if (result.first != ZONE_RELOADED) {
            return (result.first);
        }
    
        assert(result.second);
        result.second->load();
        result.second->install();
        result.second->cleanup();
    
        return (ZONE_RELOADED);
    
    as for whether we still need reload(), I don't have a strong opinion. But if we don't see specific need for it, it's probably better to clean it up (maybe later) in the sense of YAGNI. Any existing code increases maintenance costs.

It doesn't really reflect the flow of control as I see it (I think about it as „return the same thing as the getCachedZoneWriter did and use the writer if there's one“). But as this is a temporary state only probably, I don't mind, so I changed it.

I think it should be cleaned up, but the auth server still uses it now. I think
we'll remove it after we start using the background loading, since that one
will naturally switch to not use the reload() method.

memory_client.cc

  • InMemoryClient constructor: it seems to be not exception safe about zone_table_segment_. This may become a non issue if you migrate to the "formal" version of ZoneTableSegment::create, but we should check that again.

memory_client.h

  • getZoneTableSegment(): I guess we should eventually let ClientList (or at least something outside of InMemoryClient) directly manage ZoneTableSegment, at which point InMemoryClient will probably take it as a parameter to the constructor. But for now I can live with this workaround.

I guess both of these would be fixed once #2208 is merged into this, since this
code won't be present. Which should be pretty soon. If this code doesn't
disappear by then, I'll look into them.

  • not matter much, and may even become a non issue depending on what to do with reload(), but I found TYPED_TEST is often heavy in building (due to the use of template). If we keep supporting both, maybe we should consider value-parameterized test (TEST_P). It's particular so in this case because the use of different types seem to be a hack just so they can be used in TYPED_TEST.

Hmm. It seems that the size of change would not be really small and we'll
probably remove the reload() soonish. Then it would be better to just remove
the TYPED_TEST ones and unify the test fixture classes. I'd like to keep it
this way for the short time, since the time spent on compiling the templates
will definitely be smaller than the time I'd spend changing it.

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

Replying to vorner:

The new changes look good.

I think it should be cleaned up, but the auth server still uses it now. I think
we'll remove it after we start using the background loading, since that one
will naturally switch to not use the reload() method.

Then could you open a ticket for the cleanup?

memory_client.cc

  • InMemoryClient constructor: it seems to be not exception safe about zone_table_segment_. This may become a non issue if you migrate to the "formal" version of ZoneTableSegment::create, but we should check that again.

memory_client.h

  • getZoneTableSegment(): I guess we should eventually let ClientList (or at least something outside of InMemoryClient) directly manage ZoneTableSegment, at which point InMemoryClient will probably take it as a parameter to the constructor. But for now I can live with this workaround.

I guess both of these would be fixed once #2208 is merged into this, since this
code won't be present. Which should be pretty soon. If this code doesn't
disappear by then, I'll look into them.

I'm not sure about the latter, but as I said I can live with it as an
intermediate workaround. For the former, I think we need to run one
more review cycle after the cleanup. BTW, #2208 now seems to be ready
for merge.

  • not matter much, and may even become a non issue depending on what to do with reload(), but I found TYPED_TEST is often heavy in building (due to the use of template). If we keep supporting both, maybe we should consider value-parameterized test (TEST_P). It's particular so in this case because the use of different types seem to be a hack just so they can be used in TYPED_TEST.

Hmm. It seems that the size of change would not be really small and we'll
probably remove the reload() soonish. Then it would be better to just remove
the TYPED_TEST ones and unify the test fixture classes. I'd like to keep it
this way for the short time, since the time spent on compiling the templates
will definitely be smaller than the time I'd spend changing it.

If the plan is to deprecate reload(), I'm okay with keeping the
current style until then.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:13 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I think it should be cleaned up, but the auth server still uses it now. I think
we'll remove it after we start using the background loading, since that one
will naturally switch to not use the reload() method.

Then could you open a ticket for the cleanup?

I'm about to do that.

memory_client.cc

  • InMemoryClient constructor: it seems to be not exception safe about zone_table_segment_. This may become a non issue if you migrate to the "formal" version of ZoneTableSegment::create, but we should check that again.

memory_client.h

  • getZoneTableSegment(): I guess we should eventually let ClientList (or at least something outside of InMemoryClient) directly manage ZoneTableSegment, at which point InMemoryClient will probably take it as a parameter to the constructor. But for now I can live with this workaround.

I guess both of these would be fixed once #2208 is merged into this, since this
code won't be present. Which should be pretty soon. If this code doesn't
disappear by then, I'll look into them.

I'm not sure about the latter, but as I said I can live with it as an
intermediate workaround. For the former, I think we need to run one
more review cycle after the cleanup. BTW, #2208 now seems to be ready
for merge.

I tried merging it. The changes are pushed now. The merge was little bit
interesting. The merge commit contains nothing substantial, just the conflict
resolution and some slight cleanups (constification) I noticed on the way. The
adjustments to tests and the code are in follow-up commits.

I got rid of the getZoneTableSegment too.

Hmm. It seems that the size of change would not be really small and we'll
probably remove the reload() soonish. Then it would be better to just remove
the TYPED_TEST ones and unify the test fixture classes. I'd like to keep it
this way for the short time, since the time spent on compiling the templates
will definitely be smaller than the time I'd spend changing it.

If the plan is to deprecate reload(), I'm okay with keeping the
current style until then.

I think it would be the plan.

comment:14 in reply to: ↑ 13 Changed 7 years ago by jinmei

Replying to vorner:

I'm not sure about the latter, but as I said I can live with it as an
intermediate workaround. For the former, I think we need to run one
more review cycle after the cleanup. BTW, #2208 now seems to be ready
for merge.

I tried merging it. The changes are pushed now. The merge was little bit
interesting. The merge commit contains nothing substantial, just the conflict
resolution and some slight cleanups (constification) I noticed on the way. The
adjustments to tests and the code are in follow-up commits.

I got rid of the getZoneTableSegment too.

The merge and after merge changes look basically okay. I also
confirmed the previous concerns were resolved.

Some minor comments:

  • I suggest renaming DataSourceInfo::segment_ something like ztable_segment_ because "segment" can be used in several different contexts. Same for the constructor parameter, etc, too.
  • I guess we can now remove ConfigurableClientList::mem_sgmt_.
  • I'd leave the same (sense of) note about the use of assert() in the ZoneTableSegmentLocal destructor as this one:
        // leak.  assert() (with abort on failure) may be too harsh, but
        // it's probably better to find more leaks initially.  Once it's stabilized
        // we should probably revisit it.
    
    which is currently in the ConfigurableClientList destructor (and will be removed if we adopt the second suggestion)

With these I think the branch can be merged.

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:16 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 2.5

comment:17 follow-up: Changed 7 years ago by muks

Note that #2208 has simply been closed without a merge to master, expecting that this bug's merge will also include trac2208.

comment:18 in reply to: ↑ 17 Changed 7 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 2.5 to 12.8

Hello

Replying to jinmei:

The merge and after merge changes look basically okay. I also
confirmed the previous concerns were resolved.

Some minor comments:

OK, handled.

With these I think the branch can be merged.

Done.

Replying to muks:

Note that #2208 has simply been closed without a merge to master, expecting that this bug's merge will also include trac2208.

I think it would be pretty complicated to try and not include it when I
already merged it in O:-).

Note: See TracTickets for help on using tickets.