BIND 10 #1596: Addressing of multiple instances of the same component

[BIND 10 Development]

#1596: Addressing of multiple instances of the same component
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:         |           Total Hours:  0
  Multiple auths                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:
 > Hello
 >
 > Replying to [comment:6 jinmei]:
 > > - When I tried to add an additional b10-auth process (from the source
 env):
 > > [...]
 > >   with the attached config (it uses non privileged port), two
 > >   processes in fact ran, but only one of them seemed to listen on:
 >
 > That is to be expected. This is solved in #1595, which is already merged
 in master, but not in this branch yet. If you merge with master locally,
 you should see that it works (or you could trust me I tried it O:-).

 Ah, okay, I trust you:-) but I wish you could have noted it on asking
 the review.

 > > - It'd be convenient if I could differentiate different instances of
 > >   the same type of process in logs.
 >
 > Hmm, I guess it could be done somehow. But I think it would be a
 different ticket.

 That's fine.

 > > - I'm now feeling I'm confused.  What was the expected relationship
 > >   between components and processes?  [...]
 >
 > Well, when I wrote the first comment, I thought about a component that
 is a pipeline, for example, so there would be two different processes that
 do the one thing. This component could still be started multiple times by
 putting it multiple times into the configuration.
 >
 > Anyway, I'd like to be able to avoid specifying the same component
 multiple times, I'd rather have a count = something configuration element
 in each of them. If the boss then generates multiple component objects or
 just one is question for the future implementation, though. And I'd like
 to try this multiple processes thing out first a little before investing
 too much time into fine-tuning the configuration of it.

 That's fine.  But it would be helpful if the comment clarifies how a
 multi-process component would work (in future).

 > > - A related note: since this feature involves multiple processes, I
 > >   think we need system tests.  [...]
 >
 > Hmm, I'd like to merge it to master first (just to make sure both of
 them listen on the port). It could be quite easy to do the tests then,
 like starting two copies, sending a query, stopping one of them, sending a
 query, starting it again, stopping the first one (to verify the sharing of
 sockets), send the query again, then shout them both down and see that a
 query times out. So, I'd create a new ticket and put it to next-splint-
 proposed.

 That's fine for me.

 > > - what if args is bogus (e.g. pid is not int, perhaps there can be
 > >   other bogus cases)?  it should be tested.  same for resolver, and I
 > >   suspect it can be more devastating because exceptions won't be
 > >   caught at a lower level.
 >
 > Ah, you're right it could be something else than int. I check for that
 now, but I don't see any other bogus input (negative numbers would be
 handled implicitly as not equal, etc).

 See below.

 > > - do we need to regenerate and merge HTML in individual branch?
 >
 > Shouldn't I regenerate it when I change it? (it's always a problem when
 we have a generated file in the repository). Anyway, I wanted to the
 output, so I needed to regenerate it. Should I revert the html back?

 As someone who reviews the branch, it's more convenient if the diff
 only contains essential changes.  It would also help reduce conflicts
 on merge if different developers modify the guide in different
 branches.  I think this is something we should discuss at the project
 level (a candidate biweekly call item?).  For this particular branch
 I'd leave it to you whether to revert it.

 Comments on the revised branch follow:

 - auth_log.h: not a big deal (and not really specific to this ticket),
   but I wonder why we define local log levels like this:
 {{{#!c++
 // Debug messages upon shutdown
 const int DBG_AUTH_SHUT = DBGLVL_START_SHUT;
 }}}
   especially now that we have use this value for two purposes.  Why
   can't we simply use DBGLVL_START_SHUT for both cases?

 - AuthCommand::exec: in general, I'd rather prefer rejecting bogus
   inputs (either as a result of exception or by returning a specific
   error) than ignoring them.  In the case of auth, it should actually
   have been so because Element class would throw an exception (and it
   will be caught in execAuthServerCommand(), logged, and translated
   into an error answer to the command issuer).  Same comment applies
   to the resolver, although in that case we (probably) cannot simply
   throw an exception because it wouldn't be caught in a proper
   context.

 - in shutdownNotInt test: this comment isn't correct (naive
   copy-paste):
 {{{#!c++
     // With the correct PID, it should act exactly the same as in case
     // of no parameter
 }}}
   but see the previous comment.  Depending on how we do that the test
   may need to be adjusted too.

 - resover.c::my_command_handler(): it repeats a quite large comment
   that commented in auth regarding the 'not for us' note.  I'd avoid
   that and just refer from one of them to the other.  (for a longer
   term we should solve this by unifying the framework itself, but
   that's another story).

 - auth command_unittest: not a related point to this branch any more,
   but for consistency we might add the `_` postfix to other member
   variables of AuthCommandTest?

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