Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1332 closed task (complete)

Implement ZoneJournalReader class

Reported by: jinmei Owned by: jinmei
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: 5 Add Hours to Ticket:
Total Hours: 14.27 Internal?: no

Description

  • add a factory method of this class to DataSourceClient?
  • its constructor takes zone name, begin and end serials
  • it has one other method: getNextDiff()
  • implement a derived class of this new class for DatabaseClient?. it internally creates a diff iterator on construction, and getNextDiff() calls the iterator's getNext() method.

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

Subtickets

Change History (16)

comment:1 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 5

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by jelte

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

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by jinmei

trac1332 is ready for review.

It depends on #1330 (and on #1331 for writing tests), and uses
an intermediate versions of the correspoinding branches. Hopefully
the interfaces are fixed and we don't have to modify this branch
due to last minute changes to these branches. The first two commits
are for merging these branches and should be ignored.

I think the implementation itself is mostly straightforward. One thing
that may require a discussion would be how to return an error when
an unexpected input is given (non existing zone or serials). I chose
to return a result code rather than an exception because these event
are not really exceptional (an IXFR client can request a diff for
non existent version, and, depending on how we implement the application
non existent zone case can also happen at the level of this API), and
especially for C++ I believe it's better not to rely on exceptions for
normal events. But this could be a debatable point, and right now
it's not very consistent throughout the data source API, so we might
also have to discuss unifying the interface either way (but that level
of discussion will be a topic of a separate task/thread).

This is the proposed changelog entry:

317.?	[func]		jinmei
	datasrc: Added C++ API for retrieving difference of two versions
	of a zone.  A new ZoneJournalReader class was introduced for this
	purpose, and a corresponding factory method was added to
	DataSourceClient.
	(Trac #1332, git TBD)

comment:7 Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by jinmei

Forgot to mention one thing: I think we should also implement
buffering of retrieved diffs to minimize the duration of possible
database lock quite soon. But since the branch is getting large
and it's not crucial for the initial implementation of IXFR-out,
I chose not to do it within this ticket.

comment:9 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

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
    
  • The text of the exception has two prepositions in a row (for in).
        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.
  • If the comment is true, why is it a TYPED_TEST?
    @@ -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) {
    
  • The journalReader test doesn't check it successfully got any reader back or that the result is SUCCESS.
  • Is it OK with our style guidelines to check by ASSERT_TRUE and ASSERT_FALSE if the shared pointer is NULL or not?
  • 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.

Thank you

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

Replying to 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).
        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?
    @@ -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()).

comment:12 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

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:

I don't mind it, I actually find it pretty readable. I was corrected several times that I shouldn't write stuff like if (pointer), so I thought this is the same. I myself find it quite natural to evaluate a pointer in boolean context, with implicit question „is there something?“.

So, as for me, keep it the way it is. I was just wondering if you happened to overlook it as it usually happens to me.

  • 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).

No, I'm OK with the behaviour, considering that the only use we will have will probably be in IXFR-out anyway. It just didn't look too much consistent with the comment about checking. Maybe the comment could be neutralized slightly?

Anyway, I think it can be merged (with or without a change to the comment, as you see fit).

Thanks

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

Replying to vorner:

  • 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).

No, I'm OK with the behaviour, considering that the only use we will have will probably be in IXFR-out anyway. It just didn't look too much consistent with the comment about checking. Maybe the comment could be neutralized slightly?

For now I don't have a concrete idea of how to improve the wording,
so I've merged the current branch anyway. If you have a specific
suggestion that will be appreicated.

Anyway, I think it can be merged (with or without a change to the comment, as you see fit).

Okay, thanks for the review. As noted above merge done. Closing
the ticket for now.

comment:15 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 12.77

comment:16 Changed 8 years ago by vorner

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