Opened 9 years ago

Closed 8 years ago

#1068 closed task (complete)

support writability in the new data source API

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

Description

This task includes:

See commit 63e3da16e8ecdc00078ec9b0c3cb6596f3a89b60 of trac817a for
prototype implementation.

(but the actual implementation doesn't have to stick to the way that
the prototype does)

This task depends on #1060 and #1061, but otherwise can be done in
parallel with other data source refactoring tasks.

Subtickets

Change History (25)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 7

comment:2 Changed 9 years ago by stephen

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

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 9 years ago by jinmei

The first part of this taks is ready for review in the
trac1068review1 branch. It contains implementation of update related
methods of SQLite3Accessor (renamed from SQLite3Database, which is
part of refactoring change in this branch).

This branch also merged trac1183 as it seems to be the future
of the base code.

Some other notes:

  • as noted above, there are some refactoring changes in this branch that are not directly related to the topic of this task. First is to rename "database" to "accessor" in more places of the code. The other is to update/cleanup the internal details of sqlite3_accessor.cc by unifying SQL statements in SQLite3Parameters. IMO this will improve the readability quite a lot. I also removed commented out unused statements. We can always add them as we see the need for it.
  • there is an important bug fix that is actually in the master: getZone() can leave an unfinished statement. Assuming this branch can be merged not so long future, I'd rather fix it within this branch. Right now it's not used in any application, so it should be okay.
  • AddRecordColumns? and DeleteRecordParams? are almost unused right now. They will be used in the second half of the task.

comment:5 Changed 9 years ago by jinmei

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

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

As I probably had changes from 1183 in the diff as well, I might comment on something from there. But AFAIK you reviewed that one, so if it is not your code, please ignore it.

So, the comments:

  • It doesn't compile for me, because ASSERT_EQ returns and is used in the SQLite3Update constructor:
    sqlite3_accessor_unittest.cc: In constructor ‘<unnamed>::SQLite3Update::SQLite3Update()’:
    sqlite3_accessor_unittest.cc:302:60: error: returning a value from a constructor
    
    May I suggest either throwing or using EXPECT_EQ, so people will know the follow-up errors are probably related to the failure? Or maybe there's an official solution to this?
  • Comment of TTL_COLUMN = 1 is incomplete (terminating at an a).
  • Comment of ADD_TTL ‒ it says it is integer. But the actual data type is string, this wording is probably confusing. Something like „string representation of integer in decimal representation“?
  • The order of columns is different in the read and write methods (eg. the name is at the end in reading, at the beginning when writing). Wouldn't it be cleaner if they had the same order (with obvious variations around columns present only at one method)? They can be freely reordered in the SQL command, so it shouldn't be a technical problem.
  • The documentation of getRecords uses part of description for getAllRecords, which does not apply completely (for example it talks about order of RRs from different RRsets).
  • To make the interface consistent, should the vectors be replaced by fixed-sized arrays and their sizes be checked compile-time by the reference trick?
  • There are enums used as indices and with the last _COUNT item to mark the count. On one side, we have a requirement to manually number enums in our style guide (I think I saw it somewhere), but maybe in this and similar cases, we should reconsider. The standard says an enum starts at 0 and each next item is one larger than previous, if the number is free, which is exactly what we want in this case (numbering by one, starting at 0). Numbering manually is error prone, since human might just add one and not update all the previous ones, or something. Should we discuss it on the ML?
  • Isn't the agent for execution called „executioner“ instead of „executer“ (sorry, I'm not native speaker, but I did not hear the second).
  • The startUpdateZone returns an ID, but the ID isn't used to anything (not a parameter to any of the updating methods). As you note, it might not correspond to the one from getZone, so it can't be used for reading as well. What is it's purpose? I guess current API doesn't allow updating multiple zones at once (even in case the underlying DB might).
  • Is it guaranteed that the application will see the updated data in the transaction, or is it implementation detail of the SQLite one? Is it allowed for some backend to create a separate connection for the updates and have any reads out of that transaction? Do we support DBs that don't support transactions (eg. text files, CVS files, some tables of MySQL)?

With regards

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

Replying to vorner:

As I probably had changes from 1183 in the diff as well, I might comment on something from there. But AFAIK you reviewed that one, so if it is not your code, please ignore it.

So, the comments:

  • It doesn't compile for me, because ASSERT_EQ returns and is used in the SQLite3Update constructor:
    sqlite3_accessor_unittest.cc: In constructor ‘<unnamed>::SQLite3Update::SQLite3Update()’:
    sqlite3_accessor_unittest.cc:302:60: error: returning a value from a constructor
    
    May I suggest either throwing or using EXPECT_EQ, so people will know the follow-up errors are probably related to the failure? Or maybe there's an official solution to this?

Good catch, and I actually chose to simply remove any xxx_EQ. See the
commit log.

  • Comment of TTL_COLUMN = 1 is incomplete (terminating at an a).

(this is for #1183, and I pointed it out there:-)

  • Comment of ADD_TTL ‒ it says it is integer. But the actual data type is string, this wording is probably confusing. Something like „string representation of integer in decimal representation“?

Revised to "in numeric form". I see your point, and I was actually
revised the doc as suggested, but on looking into the documents
closely, this was not specific to TTL; "a domain name" could also be
confusing because it may mean a Name object or something. Some other
comments don't even talk about the intended type. The documentation
for addRecordToZone() already clarifies that these fields should be
strings. So something like "string representation of integer" seems
to be both incomplete and redundant. "in numeric form" seemed to
better address the concern while not being too redundant.

  • The order of columns is different in the read and write methods (eg. the name is at the end in reading, at the beginning when writing). Wouldn't it be cleaner if they had the same order (with obvious variations around columns present only at one method)? They can be freely reordered in the SQL command, so it shouldn't be a technical problem.

Hmm, maybe, but if we are to make the ordering consistent, I'd prefer
placing the owner name at the first column (just as in an RR); but
then it may be a bit unnatural for getNext() because the name is an
"optional" field for that method - it's expected to be untouched
(effectively "null") in case of a single lookup (via getRecords()).

Also, the ordering may not matter much as it might look; the
application would be expected to use the enum symbols rather than
directly referring to specific numbers like this.

Considering these, I'd be slightly inclined to keep this unchanged
(and let them "inconsistent"). But if you still prefer inconsistency
after taking into account these points and have a specific proposed
ordering, I'm okay with adopting it.

  • The documentation of getRecords uses part of description for getAllRecords, which does not apply completely (for example it talks about order of RRs from different RRsets).

(this is for #1183, and I pointed it out there:-)

  • To make the interface consistent, should the vectors be replaced by fixed-sized arrays and their sizes be checked compile-time by the reference trick?

Another good point. I actually considered that option, but didn't
choose it because I wanted to share the doUpdate() code for both
of the add/delete cases (they have different sizes of columns/params).
But on thinking it further, it wouldn't be a good idea to make the
public interface counter-intuitive due to the convenience of internal
implementation, and, in fact I found it not so difficult to share the
update code for both cases using C++ template. So I made the change.

  • There are enums used as indices and with the last _COUNT item to mark the count. On one side, we have a requirement to manually number enums in our style guide (I think I saw it somewhere), but maybe in this and similar cases, we should reconsider. The standard says an enum starts at 0 and each next item is one larger than previous, if the number is free, which is exactly what we want in this case (numbering by one, starting at 0). Numbering manually is error prone, since human might just add one and not update all the previous ones, or something. Should we discuss it on the ML?

Point taken. Personally I have a mixture feeling about this. On one
hand I think your point is valid. On the other hand, I don't like to
depend too much on implicit assumptions even if they are guaranteed by
the language spec - just like it's error prone to enumerate everything
by hand (I agree on that), I think it's also the case code relying on
implicit assumptions is error prone. Someone who doesn't understand
the rationale may, perhaps for a bogus reason, want to change the
initial value of the enum to non 0:

    enum DeleteRecordParams {
        DEL_NAME = 1, ///< The owner name of the record (a domain name)
                      ///< It begins with 1 as it seems to make more sense
        DEL_TYPE, ///< The RRType of the record (A/NS/TXT etc.)
        DEL_RDATA ///< Full text representation of the record's RDATA
    };

And then the code will suddenly start failing in a strange way.

In this specific it would be easy to detect and fix it via unittests,
but there may be more subtle cases. We could also add a note that the
use of the default definitions has a reason and should not be changed
in comments, but commenting something is also an error-prone human
involved practice.

So, I'm not sure which one is better. And yes, we should probably
discuss it on the ML.

  • Isn't the agent for execution called „executioner“ instead of „executer“ (sorry, I'm not native speaker, but I did not hear the second).

As you know I'm not a native English user either:-) I've looked up
dictionaries and, you're right, there's no "executer". I simply named
it so by naive convention of adding "-er", and I have no problem with
using a real word. But "executioner" doesn't seem to be the right
choice either because the only meaning (as far as I know, and
according to the dictionaries I have) of it is "a person who has the
job of executing criminals."

Maybe "StatementProcessor?" is a bit better? In any case, this is just
a private helper class only used in a single .cc, so it doesn't make
much sense to spend too much time to try to find the best name for it.
If you don't like any of the above, I'll simply go with
"StatementExecutioner?".

  • The startUpdateZone returns an ID, but the ID isn't used to anything (not a parameter to any of the updating methods). As you note, it might not correspond to the one from getZone, so it can't be used for reading as well. What is it's purpose? I guess current API doesn't allow updating multiple zones at once (even in case the underlying DB might).

Ah, yes, I should have noted that. It's currently unused, but will be
needed in the latter half of this task. It will introduce the
"Updater Finder", which is a ZoneFinder? that explores the zone being
updated (i.e., within the update transaction). If the underlying
implementation uses a different zone ID for the updated zone, the
ZoneFinder? will make sure it uses the correct ID by using the one
returned by startUpdateZone() rather than the result of getZone().

  • Is it guaranteed that the application will see the updated data in the transaction, or is it implementation detail of the SQLite one? Is it allowed for some backend to create a separate connection for the updates and have any reads out of that transaction? Do we support DBs that don't support transactions (eg. text files, CVS files, some tables of MySQL)?

As commented above, the idea is to guarantee that by the use of
"update finder", assuming the underlying database supports
transactions.

As for database (like) systems that don't support transactions, to be
honest I've not fully thought about it, but realistically I suspect we
should say the corresponding data source cannot be writable for such
systems or we should provide our own middle layer (if possible) to
emulate transactions (which will probably be a different derived class
of DatasourceClient? than the "standard" DatabaseClient?).

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • The order of columns is different in the read and write methods (eg. the name is at the end in reading, at the beginning when writing). Wouldn't it be cleaner if they had the same order (with obvious variations around columns present only at one method)? They can be freely reordered in the SQL command, so it shouldn't be a technical problem.

Considering these, I'd be slightly inclined to keep this unchanged
(and let them "inconsistent"). But if you still prefer inconsistency
after taking into account these points and have a specific proposed
ordering, I'm okay with adopting it.

It's not important, no problem in keeping them. I thought it might be just overlooked, but if there's a reason for it, it's OK.

  • To make the interface consistent, should the vectors be replaced by fixed-sized arrays and their sizes be checked compile-time by the reference trick?

Another good point. I actually considered that option, but didn't
choose it because I wanted to share the doUpdate() code for both
of the add/delete cases (they have different sizes of columns/params).
But on thinking it further, it wouldn't be a good idea to make the
public interface counter-intuitive due to the convenience of internal
implementation, and, in fact I found it not so difficult to share the
update code for both cases using C++ template. So I made the change.

Yes, nice. Well, it could be done with std::string param[] (which would take whatever length of array), but it doesn't matter.

  • There are enums used as indices and with the last _COUNT item to mark the count. On one side, we have a requirement to manually number enums in our style guide (I think I saw it somewhere), but maybe in this and similar cases, we should reconsider. The standard says an enum starts at 0 and each next item is one larger than previous, if the number is free, which is exactly what we want in this case (numbering by one, starting at 0). Numbering manually is error prone, since human might just add one and not update all the previous ones, or something. Should we discuss it on the ML?

[...]

So, I'm not sure which one is better. And yes, we should probably
discuss it on the ML.

ACK, I'll send an email.

  • Isn't the agent for execution called „executioner“ instead of „executer“ (sorry, I'm not native speaker, but I did not hear the second).

As you know I'm not a native English user either:-) I've looked up
dictionaries and, you're right, there's no "executer". I simply named
it so by naive convention of adding "-er", and I have no problem with
using a real word. But "executioner" doesn't seem to be the right
choice either because the only meaning (as far as I know, and
according to the dictionaries I have) of it is "a person who has the
job of executing criminals."

Well, maybe the dictionary has execute only as cut off someone's head. But I admit the executioner probably isn't usually used as something that executes code (although executing code is repeatedly removing the head of the code and doing something with it) and this use would be somewhere on borderline with language joke.

The StatementProcessor? is probably slightly better, but anything is OK if the reader can guess what it does. I just noted the word looked odd to me.

  • Is it guaranteed that the application will see the updated data in the transaction, or is it implementation detail of the SQLite one? Is it allowed for some backend to create a separate connection for the updates and have any reads out of that transaction? Do we support DBs that don't support transactions (eg. text files, CVS files, some tables of MySQL)?

As commented above, the idea is to guarantee that by the use of
"update finder", assuming the underlying database supports
transactions.

ACK, I just asked.

It can be merged (or considered done), with or without the renaming, as you like.

With regards

comment:11 in reply to: ↑ 10 Changed 8 years ago by jinmei

Replying to vorner:

It can be merged (or considered done), with or without the renaming, as you like.

Okay, I've changed the helper class name and merged it to master.

I'll keep the ticket open for the second half of the task.

Thanks for the review so far.

comment:12 Changed 8 years ago by jinmei

The second part of this task is ready for review. It's on branch
trac1068review2.

A couple of notes:

  • I wanted to make one other set of changes: making DatabaseClientTest? polymorphic and share the tests for sqlite3, too. I have code for that, but it would probably make the entire diff too large, so I decided not to include it this time. That will be the next (and final) round of the review process.
  • the change to rrsig_46 is completely unrelated to the task. I happened to notice we can/should make this change when I tried to use it, and since it was very small I piggy-backed it on this branch.

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned

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

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

Do you think it would be a problem to create another branch (not necessarily a ticket) for the rest you already have and push it right away? They could be reviewed in parallel while still keeping the code change small per one branch/diff.

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

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

Hello

First, notes about the interface ‒ I think it might be useful to be able to add and remove zones in backend-agnostic way (and maybe as an atomic operation with filling it with data). For example b10-loadzone could use it. But it might be out of the scope of this ticket.

Then, of course, few notes about the code:

  • The underlying realization of a "transaction" will defer for different ‒ you mean „differ“ or that we defer it to the different implementation?
  • This isn't something that would be wrong in any way and it is possibly only style matter, but I'm just curious about the rationale about it ‒ the DatabaseUpdater?'s methods (addRRset, deleteRRset, commit) live outside the class definition. Why is it so? I think that the original reason for being able to have method implementation outside the class exists because of class lives in header file and the code shouldn't, but that doesn't apply as both are in the source in this case.
  • Should addRRset take the attached signature into account as well? (eg. issue the th addRRset on the signature if it's present, or something)
  • Why are add_columns_ and del_params_ object-level variables? It would be enough for them to live only inside the methods they are used in, wouldn't it?
  • Related to this: In case the RRSIG isn't the last thing added through this updater, any subsequent RRs will have the same ADD_SIGTYPE column. Should it be reset afterwards to empty string?
  • Can the destructor of the accessor throw? If so, the commit would have a problem ‒ the committed_ would stay as false and the class will call rollback on the (already destroyed or partly destroyed) accessor. Should the committed_ = true be moved above the accessor_.reset()?
  • A style issue ‒ the commit and rollback are handled from within the updater. Should startUpdateZone be called from the DatabaseUpdater?'s constructor instead of outside the class, because of consistency?
  • This is probably out of the scope of this ticket, but shouldn't the getUpdater of in-memory propagate the call to the underlying data source instead of calling NotImplemented? in the long term? If so, maybe there should be a note/comment about it?
  • The protected ZoneUpdater? constructor is not needed. If anybody tries to create instance of it, the compilation will fail because it has pure virtual methods.
  • Would it help to have a test for „bigger“ transaction (like more adds and deletes during one transaction)?

comment:16 Changed 8 years ago by vorner

Ah, and I forgot to mention ‒ I pushed two small changes, one fixes compilation for me (a name of constructor was used instead of the name of type) and the other is just indentation fix. I hope they are OK.

comment:17 in reply to: ↑ 15 Changed 8 years ago by jinmei

Replying to vorner:

First, notes about the interface ‒ I think it might be useful to be able to add and remove zones in backend-agnostic way (and maybe as an atomic operation with filling it with data). For example b10-loadzone could use it. But it might be out of the scope of this ticket.

I was aware of the need (and the lack in the current implementation)
for adding/removing zones. I also think we should move the
functionality of creating a new DB outside the basic responsibility of
the accessor (in fact it does something beyond the scope of its
work). But as you thought I think these extensions/cleanups would
better be deferred to later plans.

Then, of course, few notes about the code:

  • The underlying realization of a "transaction" will defer for different ‒ you mean „differ“ or that we defer it to the different implementation?

Ah, good catch. Yes, it should be "differ". Fixed.

  • This isn't something that would be wrong in any way and it is possibly only style matter, but I'm just curious about the rationale about it ‒ the DatabaseUpdater?'s methods (addRRset, deleteRRset, commit) live outside the class definition. Why is it so? I think that the original reason for being able to have method implementation outside the class exists because of class lives in header file and the code shouldn't, but that doesn't apply as both are in the source in this case.

It's a subjective decision (or preference). I wanted to keep the size
of the class definition reasonably concise so that the entire
definition can be browsed at a glance (e.g., without scrolling on a
reasonably larger size of window). So I moved some relatively larger
methods outside the definition. On the other hand I admit it may look
inconsistent as some others are still in the class definition even if
they are super simple ones like getFinder(), and the conciseness
argument may not be that convincing anyway if we want to provide more
inline comments. So this is not a strong preference.

  • Should addRRset take the attached signature into account as well? (eg. issue the th addRRset on the signature if it's present, or something)

May or may not be. As we briefly talked before in a different context,
I personally wanted to revisit the design of how to represent signed
RRsets, and would hesitate to make the interface "too convenient"
until then (including the possible conclusion of not changing the
design).

As far as I can see we shouldn't need to allow this mode in the
meantime, so I believe we can safely defer it to later discussions.
But to prevent accidental misuse I added explicit checks so that
an attempt of adding/deleting RRset + RRSIG RRset will result in an
exception (for the delete case it would actually make sense to
prohibit it).

  • Why are add_columns_ and del_params_ object-level variables? It would be enough for them to live only inside the methods they are used in, wouldn't it?
  • Related to this: In case the RRSIG isn't the last thing added through this updater, any subsequent RRs will have the same ADD_SIGTYPE column. Should it be reset afterwards to empty string?

Ah, good point. As for why as member variables, they were originally
vectors, and I wanted to avoid the overhead of constructing them
every time the methods are called (but even then it might be a bad
attempt of premature optimization - after all these methods are not
expected to be performed in a very performance sensitive path). Now
they are plain arrays, it makes even more sense to define them locally
in the methods.

As for SIGTYPE, yes, you're right, the original code had a bug.

I updated the code to define the arrays as method local, and (although
it's now pretty obvious to be safe from the code) added a test case
that would have identified the bug in the previous version.

  • Can the destructor of the accessor throw? If so, the commit would have a problem ‒ the committed_ would stay as false and the class will call rollback on the (already destroyed or partly destroyed) accessor. Should the committed_ = true be moved above the accessor_.reset()?

Hmm, good point. I'd call it a bug if the destructor of an accessor
ever threw (it would cause other serious effects even if not here),
but since the accessor API is public and we expect external developers
to write their own accessors possibly with bugs, it would be better to
be as proactive as possible. So I changed the code as suggested.

  • A style issue ‒ the commit and rollback are handled from within the updater. Should startUpdateZone be called from the DatabaseUpdater?'s constructor instead of outside the class, because of consistency?

I have no strong opinion on for/against the consistency per se, but it
wouldn't be that straightforward in this case. According to the
current API design we need to return a NULL pointer if the specified
zone doesn't exist. This condition is checked via the return value
from startUpdateZone(), so if we included the call in the
DatabaseUpdater? constructor, we'd need to have it throw for
non-existent zone, catch it in getUpdater (and differentiate it from
other errors resulting in the same exception type), and convert it to
a NULL pointer.

Since the DatabaseUpdater?() class details are purely internal to the
database.cc implementation, I think we can be a bit more flexible on
the consistency on the interfaces. So, right now I've not modified
the code on this point, preferring implementation simplicity.

  • This is probably out of the scope of this ticket, but shouldn't the getUpdater of in-memory propagate the call to the underlying data source instead of calling NotImplemented? in the long term? If so, maybe there should be a note/comment about it?

Possibly, or possibly not. Right now it's not clear how we relate the
in-memory client to its underlying databases (or some other backend
storage) and how applications that need to get access to the
underlying storage directly (e.g. whether it's via the in-memory
interface or not).

  • The protected ZoneUpdater? constructor is not needed. If anybody tries to create instance of it, the compilation will fail because it has pure virtual methods.

I think we discussed this before - in the current implementation,
that's true, but I personally prefer making this explicit. IMO it's
fragile to rely on the existence of a pure virtual method (without a
definition) for this purpose; we may want to revise the interface to
provide the default implementation at the base class level. We could
still provide the default with keeping it pure virtual by defining the
pure virtual function and having derived classes call it explicitly,
but we are not currently using that practice. Also, while the same
argument could apply to the constructor (we might want to make it
public for some reason), I believe it's much less common in practice.

If you still don't like the redundancy in explicitly declaring and
defining the constructor or do want to have a unified policy, I'm okay
with having a discussion project wide. Right now I'm keeping the
current code.

  • Would it help to have a test for „bigger“ transaction (like more adds and deletes during one transaction)?

Good idea. Added a new test named compoundUpdate.

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

Replying to vorner:

Do you think it would be a problem to create another branch (not necessarily a ticket) for the rest you already have and push it right away? They could be reviewed in parallel while still keeping the code change small per one branch/diff.

I've held off as the added tests as a result of the review for review2
would affect the final subtask. Now I've merged the latest review2
and made necessary updates, I've published the last bit as
trac1068review3 in the central repo.

The point of the changes in this branch is quite straightforward,
although the amount of the change may look big:

  • moved the test records outside the mock accessor so that we can share it with other accessors
  • use gtest's TYPED_TEST to share most of the test cases. Due to the characteristics of the TYPED_TEST (see the gtest documentation) we need to explicitly add "this->" to test class member variables. This is one main reason for the bigger changes. Some tests only work with the mock accessor, so I added some workaround to do tests selectively depending on the accessor type.

Assuming the interface of the updater API is fixed, this branch could
be reviewed by anyone. But realistically it would be much less
efficient if someone who never saw update tests did it, so I'm keeping
it in this ticket and assume you'll take on it (I thank you for it in
advance but if you think otherwise please say so, then I'll consider
making a sub ticket).

comment:19 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I was aware of the need (and the lack in the current implementation)
for adding/removing zones. I also think we should move the
functionality of creating a new DB outside the basic responsibility of
the accessor (in fact it does something beyond the scope of its
work). But as you thought I think these extensions/cleanups would
better be deferred to later plans.

ACK

I have no strong opinion on for/against the consistency per se, but it
wouldn't be that straightforward in this case. According to the
current API design we need to return a NULL pointer if the specified
zone doesn't exist. This condition is checked via the return value
from startUpdateZone(), so if we included the call in the
DatabaseUpdater? constructor, we'd need to have it throw for
non-existent zone, catch it in getUpdater (and differentiate it from
other errors resulting in the same exception type), and convert it to
a NULL pointer.

Yes, I see. I didn't notice that before, so let's keep it like it is.

  • The protected ZoneUpdater? constructor is not needed. If anybody tries to create instance of it, the compilation will fail because it has pure virtual methods.

I think we discussed this before - in the current implementation,
that's true, but I personally prefer making this explicit. IMO it's
fragile to rely on the existence of a pure virtual method (without a
definition) for this purpose; we may want to revise the interface to
provide the default implementation at the base class level. We could
still provide the default with keeping it pure virtual by defining the
pure virtual function and having derived classes call it explicitly,
but we are not currently using that practice. Also, while the same
argument could apply to the constructor (we might want to make it
public for some reason), I believe it's much less common in practice.

I should try to remember the reason.

Anyway, this part is OK to be merged (or you can wait for the rest to finish being reviewed).

Replying to jinmei:

  • use gtest's TYPED_TEST to share most of the test cases. Due to the characteristics of the TYPED_TEST (see the gtest documentation) we need to explicitly add "this->" to test class member variables. This is one main reason for the bigger changes. Some tests only work with the mock accessor, so I added some workaround to do tests selectively depending on the accessor type.

It looks OK. But I'm thinking about one thing ‒ there are several tests that are skipped with different types than the MockAccessor? (like nullIteratorContext). Should we take them out of the typed tests and do something like this? So they wouldn't show up in the outputs when they are effectively empty.

typedef DatabaseClientTest<MockAccessor> MockDatabaseTest;

TYPE_F(MockDatabaseTest, nullIteratorContext) {

}

With regards

comment:21 in reply to: ↑ 20 Changed 8 years ago by jinmei

Replying to vorner:

Anyway, this part is OK to be merged (or you can wait for the rest to finish being reviewed).

Okay, thanks. I'll push this part soon anyway. Other tasks depend on this
(the last part is only for tests on which no one depends), so it would be
more convenient to be in the master.

It looks OK. But I'm thinking about one thing ‒ there are several tests that are skipped with different types than the MockAccessor? (like nullIteratorContext). Should we take them out of the typed tests and do something like this? So they wouldn't show up in the outputs when they are effectively empty.

Hmm, I personally don't care much about it showing up multiple times
(and would prefer avoiding additional tricks), but I also realize it
makes the body of the test code simpler by omitting the need for
checking is_mock_. So I made the suggested change.

comment:22 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Thanks, looks OK. Please merge.

comment:24 in reply to: ↑ 23 Changed 8 years ago by jinmei

Replying to vorner:

Thanks, looks OK. Please merge.

Merge done, closing ticket. Thanks for the review.

comment:25 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.