Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1331 closed task (complete)

Extend the ZoneUpdater class to support adding diffs

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20111122
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket:
Total Hours: 13.17 Internal?: no

Description

  • add a new parameter 'journaling' to the constructor
  • update DatabaseUpdater? so that it calls DatabaseAccessor::addDiff() when 'journaling' is true on construction. (it should probably check if the given sequence of diffs meet the assumption, too)

Subtickets

Attachments (1)

trac1331.diff (9.9 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 years ago by jinmei

This task depends on #1329, but with a mock class for tests
it could be handled separately.

comment:2 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 4

comment:3 Changed 8 years ago by jelte

  • Add Hours to Ticket 4 deleted
  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by vorner

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

comment:6 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:7 Changed 8 years ago by vorner

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

Hello

It should be ready for review. I needed to add SOA::getSerial method (the SOA doesn't have any getter methods, but I didn't need any other, so I didn't include them in this ticket).

I'm not sure if it's the best thing to do, but if the underlying data source doesn't support the diffs, it silently not stores them. Should it throw instead or something?

I don't think this needs a changelog entry, as this isn't user visible at all.

Thank you

comment:8 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:9 Changed 8 years ago by jinmei

general/design

  • I personally think we should notify the caller of an explicit failure if journaling is true while the underlying data source doesn't support it.

We should eventually support the capabilities (there's a ticket),
and once it's supported the application will be expected to first
check the capability of the data source and set 'journaling'
accordingly. (otherwise, things like dynamic DNS won't work
correctly if, for example, the underlying data source is volatile
like writable in-memory without a persistent journal system; the
application would at least rely on the journal to be stored in a
persistent storage and return a successful response to a dynamic DNS
update request once updater.commit() succeeds. If the underlying
data source actually doesn't support it and simply ignore the
mismatch, an unexpected thing will happen due to e.g. power
failure).

With this scenario in mind, the application shouldn't even try to
use journaling unless it knows it's supported, and, if some strange
reason addRRset() or deleteRRset() fails due to diff handling
failure even the capability check suggests it's supported, the
application should actually know that. At the very least I believe
we should log the event here

            // Don't fail if the backend can't store them
            catch(const isc::NotImplemented&) {}

Silently discarding an internal failure is generally not a good
practice.

  • I'm not sure if we should allow this flexibility: "When the journaling is true, it *might* require that the following update be formated as IXFR" (as documented in client.h), and I'd rather suggest it be required without may/might. Unless/until we have a smart data source backend that can automatically figure out the correct ordering internally *and* the application selectively wants to use that particular data source, this "may" will simply increase the possibility of giving a false sense of flexibility to applications, which subsequently leads to bugs.
  • As for changelog, I personally don't have an opinion, but considering Jeremy wanted to have a changelog entry for #1329 (which is even more invisible for most users/developers than this one), I suspect he'd also want to have it for this task.

zone.h

Maybe in the class description, I suggest adding a reference to
DataSourceClient::getUpdater for the meaning of "IXFR sequence".

database.cc

I've not found any substantial issues (except for things related to
the higher level design). There are some things to discuss though:

  • while quite straightforward, I'm afraid that addRRset() and deleteRRset() are now getting bigger and decreasing readability. I propose something like the attached diff. This patch actually contains one (unrelated) bug fix: reject an empty RRset at an earlier point of validation; in the previous code if an empty RRset is passed and rejected by other validation check, RRset::toText() would cause another disruption.
  • 'rdata::' could be omitted here:
                serial_ =
                    dynamic_cast<const rdata::generic::SOA&>(it->getCurrent()).
                    getSerial();
    
    (there are two such cases)
  • need a space after catch:
                // Don't fail if the backend can't store them
                catch(const isc::NotImplemented&) {}
    
    • also, I've been noticing style inconsistency on whether to place catch in the same line of the closing brace for try.
    • but in this particular case I'd rather not catch the exception here in the first place as already commented.
  • Unexpected doesn't seem to be an appropriate exception here:
            isc_throw(isc::Unexpected, "Update sequence not complete");
    
    This exception was generally intended to be used for internal unexpected events. In this case this is basically an application bug, so we should use something else. Maybe abusing BadValue even though it's not about the parameter, or introduce something like BadOperation, or some new data source specific exception.

database_unittest.cc

  • I've fixed a build error on my system (actually I was surprised it compiled on other systems) and pushed it. commit 6da32ea.
  • This code seems to be a bit cryptic to me:
            for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
                data_equal = data_equal && (data_[i] == other.data_[i]);
            }
    
    While I generally love the elegance of a concise statement, this particular one made me use more brain cycle to understand, perhaps because it's used in the context of traditional procedural style. While it may look dumb, I'd rather prefer something like this:
            for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
                if (data_[i] != other.data_[i]) {
                    data_equal = false;
                    // we could even break here;
                }
            }
    
    If you really want to keep the original style, I won't fight against it further, but I'd at least suggest adding redundant parentheses for readability:
                data_equal = (data_equal && (data_[i] == other.data_[i]));
    

Changed 8 years ago by jinmei

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

forgot to mention this: do we want to check when an SOA RRset is added/deleted
it has exactly one RDATA?

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

  • Owner changed from vorner to jinmei

Hello

Everything should be addressed except:

Replying to jinmei:

forgot to mention this: do we want to check when an SOA RRset is added/deleted
it has exactly one RDATA?

Well, I don't think such change should be related to this branch. We might want to check that, and check that CNAME is the only one and so on…

This branch wouldn't be the only thing that would act strange if there are multiple SOA RRs (considering they would have different serial, if they were the same, this branch wouldn't mind). But if you think it should, I have no problem adding the check. Anyway, I think this should be checked on some different level, because we can't do a complete check anyway (because add „merges“ with existing data anyway).

Thanks

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

About the latest code:

  • There was a typo that breaks compile. Fixed and pushed.
  • In zone.h, is this "...look like an IXFR"?
        /// If journaling was requested when getting this updater, it might reject
        /// to add the RRset if the squence doesn't look like and IXFR (see
    
    (didn't notice that in the previous review). Also, I guess we should now change "might reject" to "will reject". Applies to both addRRset() and deleteRRset().

Replying to jinmei:

forgot to mention this: do we want to check when an SOA RRset is added/deleted
it has exactly one RDATA?

Well, I don't think such change should be related to this branch. We might want to check that, and check that CNAME is the only one and so on…

This branch wouldn't be the only thing that would act strange if there are multiple SOA RRs (considering they would have different serial, if they were the same, this branch wouldn't mind). But if you think it should, I have no problem adding the check. Anyway, I think this should be checked on some different level, because we can't do a complete check anyway (because add „merges“ with existing data anyway).

I don't necessarily think it should; I just wondered. But the reason
about this is different from other semantics errors like duplicate
CNAME. In the case of multiple SOA RRs, it would break the specific
assumption of this method, that is, the resulting sequence is in the
IXFR order.

BIND 9 does some other consistency checks such as whether serials are
increasing (duplicate SOA is impossible in BIND 9 due to the API
restriction).

I simply don't know how much we want to check here. At least the
duplicate SOA case shouldn't happen via ixfr-in, and the dynamic
update server will probably ensure that doesn't happen itself, so
maybe we can defer the decision until we see the real need for it.
I'd leave it to you.

Finally, in case you forget it, please check with Jeremy whether we
want to have a changelog entry.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • In zone.h, is this "...look like an IXFR"?
        /// If journaling was requested when getting this updater, it might reject
        /// to add the RRset if the squence doesn't look like and IXFR (see
    
    (didn't notice that in the previous review). Also, I guess we should now change "might reject" to "will reject". Applies to both addRRset() and deleteRRset().

Hmm, but then if someone would like to implement their own data source client, they would be required to check it. I'd like to allow the flexibility in not requiring the check. Any idea how to word it so it is clear the sequence must be IXFR like, but the check is optional?

I don't necessarily think it should; I just wondered. But the reason
about this is different from other semantics errors like duplicate
CNAME. In the case of multiple SOA RRs, it would break the specific
assumption of this method, that is, the resulting sequence is in the
IXFR order.

It is harder to create a SOA with multiple RRs than it is to mix the order. So I'd say we leave this check for now, it doesn't seem to bring any real advantage.

Finally, in case you forget it, please check with Jeremy whether we
want to have a changelog entry.

He says he would like one. So, something like this?

[func]
The database data sources can now store diffs when updating the data.

Thanks.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

  • In zone.h, is this "...look like an IXFR"?
        /// If journaling was requested when getting this updater, it might reject
        /// to add the RRset if the squence doesn't look like and IXFR (see
    
    (didn't notice that in the previous review). Also, I guess we should now change "might reject" to "will reject". Applies to both addRRset() and deleteRRset().

Hmm, but then if someone would like to implement their own data source client, they would be required to check it. I'd like to allow the flexibility in not requiring the check. Any idea how to word it so it is clear the sequence must be IXFR like, but the check is optional?

If the sequence requirement is a must, what's the point of the
"flexibility" of not doing the check? That just seems to be laziness
that naively trusts the application does the right thing.

If you are thinking about future extension of a smarter data source
(which we discussed in this ticket) with a specific application that
selectively uses that kind of data sources, we'll of course loosen the
check requirement at that point. But until/unless we have a real need
for it I'd rather keep the check tighter so that bugs in the
application can be detected more easily and sooner.

I don't necessarily think it should; I just wondered. But the reason
about this is different from other semantics errors like duplicate
CNAME. In the case of multiple SOA RRs, it would break the specific
assumption of this method, that is, the resulting sequence is in the
IXFR order.

It is harder to create a SOA with multiple RRs than it is to mix the order. So I'd say we leave this check for now, it doesn't seem to bring any real advantage.

Finally, in case you forget it, please check with Jeremy whether we
want to have a changelog entry.

Okay.

He says he would like one. So, something like this?

[func]
The database data sources can now store diffs when updating the data.

We actually also extend the top level interface. I'd mention that,
e.g.:

317.?	[func]
	datasrc: the getUpdater (and get_updater for python) method of
	DataSourceClient supports an optional 'journaling' parameter to
	indicate the generated updater to store diffs.  The database based
	derived class implements this extension.

comment:17 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

If the sequence requirement is a must, what's the point of the
"flexibility" of not doing the check? That just seems to be laziness
that naively trusts the application does the right thing.

As discussed on jabber, my intention was to allow the lazy backends, but as discussed, I strengthened the requirement and it „will“ instead of „might“ now.

I also discovered I forgot to commit a test change, so it got lost. It is there now.

He says he would like one. So, something like this?

[func]
The database data sources can now store diffs when updating the data.

We actually also extend the top level interface. I'd mention that,
e.g.:

317.?	[func]
	datasrc: the getUpdater (and get_updater for python) method of
	DataSourceClient supports an optional 'journaling' parameter to
	indicate the generated updater to store diffs.  The database based
	derived class implements this extension.

Is the python interface update supposed to be part of this ticket? If so, I should add it, because this ticket handles the C++ part only.

comment:19 in reply to: ↑ 18 Changed 8 years ago by jinmei

Replying to vorner:

If the sequence requirement is a must, what's the point of the
"flexibility" of not doing the check? That just seems to be laziness
that naively trusts the application does the right thing.

As discussed on jabber, my intention was to allow the lazy backends, but as discussed, I strengthened the requirement and it „will“ instead of „might“ now.

I also discovered I forgot to commit a test change, so it got lost. It is there now.

Okay, I think it's now ready for merge.

Is the python interface update supposed to be part of this ticket?

No, it's for a separate ticket (#1333).

comment:20 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:21 Changed 8 years ago by vorner

  • Add Hours to Ticket set to 9
  • Resolution set to complete
  • Status changed from reviewing to closed

comment:22 Changed 8 years ago by vorner

  • Add Hours to Ticket 9 deleted
  • Total Hours changed from 0 to 9

comment:23 Changed 8 years ago by jinmei

  • Total Hours changed from 9 to 13.17
Note: See TracTickets for help on using tickets.