BIND 10 #1332: Implement ZoneJournalReader class

[BIND 10 Development]

#1332: Implement ZoneJournalReader class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111122
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:         |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 vorner]:

 > There are few notes I have:
 >  * I believe the correct word is „track“, not „truck“
 > {{{
 > implementation is assumed to keep truck of sufficient history to
 > }}}

 Ah, yes, correct.  A typo only a non native speaker could do:-)  Fixed.

 >  * The text of the exception has two prepositions in a row (for in).
 > {{{#!c++
 >     virtual std::pair<ZoneJournalReader::Result, ZoneJournalReaderPtr>
 >     getJournalReader(const isc::dns::Name&, uint32_t, uint32_t) const {
 >         isc_throw(isc::NotImplemented, "Journaling isn't supported for "
 >                   "in Nop data source");
 >         //return (ZoneJournalReaderPtr());
 >     }
 > }}}
 >  * Why is the return left there commented out? I guess it should be
 removed.

 Fixed both.  The commented-out line was just a leftover.

 >  * If the comment is true, why is it a TYPED_TEST?
 > {{{#!diff
 > @@ -2870,76 +2969,79 @@ TEST_F(MockDatabaseClientTest, journalMultiple)
 {
 >   *
 >   * Note that we implicitly test in different testcases (these for add
 and
 >   * delete) that if the journaling is false, it doesn't expect the
 order.
 > + *
 > + * In this test we don't check with the real databases.
 >   */
 > -TEST_F(MockDatabaseClientTest, journalBadSequence) {
 > +TYPED_TEST(DatabaseClientTest, journalBadSequence) {
 > }}}

 Good catch, this could actually be TEST_F, so I reverted it with some
 more explanation about why.

 And this reminded me of another thing: I originally planned to extend
 checkJournal so that it can support other databases too.  I forgot it
 in the initial push, so I now added it.  These changes are at commits
 93a5d45
 and fdefb47.

 >  * The journalReader test doesn't check it successfully got any reader
 back or that the result is SUCCESS.

 Ah, okay, added.

 >  * Is it OK with our style guidelines to check by ASSERT_TRUE and
 ASSERT_FALSE if the shared pointer is NULL or not?

 This actually used the type conversion operator of the shared pointer
 class,
 and in that sense it's something not covered by the style guideline.

 I don't have a strong opinion about how to do this in this case, and
 I actually don't like relying on implicit type conversions too much,
 but in this case alternatives don't seem to be attractive either:
 {{{
     // not work on some system (require static_cast), and it also relies
 on
     // type conversion anyway.
     ASSERT_EQ(NULL, rrset);
     // this looks a bit awkward
     ASSERT_EQ(ConstRRsetPtr(), rrset);
     // this probably works without static_cast, but still relies on type
     // conversion
     ASSERT_TRUE(rrset != NULL);
 }}}

 If you have a strong opinon or preference, I'm okay with changing it
 to that one.  Also, we might want to uniffy the style about the type
 conversion for the shared pointer class in general.

 >  * The database is expected to hold if a given diff is addition or
 removal. Why doesn't the interface provide this information? One comment
 says that an application might want to explicitly check that it is correct
 IXFR sequence, but it can't really do so without this information.

 Because it's basically part of the interface contracts (i.e., if it
 doesn't hold it's a bug of the underlying implementation or someone
 intentionally tweak the data by hand).  An applicatoin can check (if
 it really wants) the sequence is valid just like when xfrin checks the
 sequence is valid from an IXFR response (which doesn't contain an
 explicit flag of add or delete).

 It also follows the convention of BIND 9's journal interface (not
 necessarily to say it's an unbreakable norm, though):
 {{{
 void
 dns_journal_current_rr(dns_journal_t *j, dns_name_t **name, isc_uint32_t
 *ttl,
                        dns_rdata_t **rdata);
 }}}
 This is an equivalent of our getNextDiff(), and name/ttl/rdata are the
 return values from the journal; it doesn't specify whether it's to add or
 delete.

 Again, if you have a strong opinion, we can discuss it (note that
 we'll also need to update the accessor interface if we want to include
 the information of the operation type (add/delete) in the return
 value of getNextDiff()).

-- 
Ticket URL: <http://bind10.isc.org/ticket/1332#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development