BIND 10 #1198: Split DatabaseClient::Finder::find

[BIND 10 Development]

#1198: Split DatabaseClient::Finder::find
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20111220
                   Priority:  minor  |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  High   |           Total Hours:  0
Feature Depending on Ticket:         |
  Datasrc refactoring                |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 > I agree the added comments help understand what's happening (and in some
 specific cases why), and they are very good for that purpose. But at the
 same time I'm afraid the massive amount of in-method comments rather
 reduces readability at a glance and would make it more difficult to extend
 the code later with confidence. For example, if we see the following
 organization...
 Here I have to disagree with you.  I find it more awkward to have to
 bounce back and forth between the header and the code to decide what is
 happening.  I think the header should be for the general overview of what
 the function does, perhaps with details of the algorithm.  But if that
 gets too detailed, it becomes pseudocode. (Also, the function header is
 usually put in the .h file, so is separate from the code.)

 > Now, imagine that each comment provides very detailed explanation of
 what is going on each case and why we do that, etc, and consumes the half
 of "vertical screen size". Even if the essential code logic is trivial,
 we'd need to scroll up and down to check whether it's really safe to add
 code at the very end part of the function. I'm afraid that's happening in
 the revised code of findWildcardMatch() and the main find() method.
 I think that if you are not clear what is happening - and very
 importantly, why it is happening - because of insufficient detail, it is
 probably unsafe to add code anyway.

 However...

 I think we are both agreed on the need for adequate documentation, we just
 have a different opinion of where it should be put.  (I mention in passing
 that a number of reviews have taken me a long time because I had to
 decipher what is going on to understand whether the code being added was
 relevant and correct - something that would have been considerably eased
 with better documentation.) In the end though it doesn't really matter
 where the documentation is, providing it is there; so if you prefer to
 have all the documentation in the header, I'm fine with that.

 Regarding the other changes (findOnNameResult() etc.) they look fine.
 However, when I tried to apply the diffs file I got a number of rejections
 - not all changes were applied.  So I have not applied any.


 > '''reader_message_file.py'''[[BR]]
 > removeEmptyLeadingTrailing and processFile should be named
 remove_empty_leading_trailing and process_file?
 They should and have been changed.  (And I'm mortified about that! - I
 picked someone else up on that very point in a review a few days ago.  A
 question of "do as I say" and not "do as I do" I'm afraid :-$)

 '''findWildcardMatch'''[[BR]]
 > Personally, I'd use assert for the "sanity check" for FIND_GLUE_OK
 because should this happen it's a pure internal bug within database.cc
 (even a broken database table cannot make this happen).
 True, but if it ever happens in the wild due to a subsequent change to the
 code, we end up with the server stopping and a cryptic abort message.  At
 least with an exception (a) the problem is reported through the normal
 logging channels, (b) it is not disabled if NDEBUG is set and (c) we have
 to opportunity to take remedial action, e.g. abort the current query.
 That way we could at least run a degraded service instead of no service.

 > Why are there two blank lines here?
 Over-enthusiasm about the aesthetics of the code layout.  (Either that or
 a mistake.)  Spurious blank line removed.

 > I'm not sure "not exist in the zone" is correct, because a formal
 definition of a "zone" could be the tree. I'd say "not in the database".
 Changed.

 > I'd remove this comment. I believe the detailed comment before this
 provides sufficient explanation.
 Removed.

 > I suggest revising the !NO_WILDCARD case as follows:
 Good catch, changed.

 > DNAME applies only to subdomains of the DNAME's owner name. (Remember
 the whole chaos of BNAME, etc discussion. One reason for that is this
 property of DNAME).
 Comment updated.

 > Regarding the note about the "resolver":
 Note modified.  I felt it important to explain why we terminate the search
 even though we were not searching for a CNAME.

 > As for this "TODO":
 Thanks for the explanation and sanity check, TODO comment removed.

 > '''datasrc_messages.mes'''
 Changes applied.

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