BIND 10 #352: Improve Xfrout request handling strategy

[BIND 10 Development]

#352: Improve Xfrout request handling strategy
------------------------------+---------------------------------------------
      Reporter:  zzchen_pku   |        Owner:  zzchen_pku
          Type:  enhancement  |       Status:  reviewing 
      Priority:  major        |    Milestone:            
     Component:  xfrout       |   Resolution:            
      Keywords:               |    Sensitive:  0         
Estimatedhours:  0.0          |        Hours:  0         
      Billable:  1            |   Totalhours:  1.5       
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by zhanglikun):

 Replying to [comment:2 jinmei]:
 > Replying to [ticket:352 zzchen_pku]:
 > > Xfrout server polls for shutdown every 0.1 seconds. Polling reduces
 Xfrout server's responsiveness to a shutdown request and wastes cpu at all
 other times. Consider using interrupt-driven instead of polling.
 >
 > The idea looks good, but I have some possibly substantial comments about
 the implementation.
 >
 > '''serve_mixin.py'''
 >  - I think we need more documentation about this class.  there seem to
 be many hidden assumptions, which are not clear from the code itself and
 the current documentation.  For example, it seems this class is assumed to
 be used with socketserver.ThreadingMixIn, but it's not clear.  It's also
 not clear whether we expect to have multiple (derived) instances of this
 mixin class.  Also non obvious is what we'd expect with poll_interval,
 which is unused.  Overall, we need to descrive the intention of the class,
 expected usage, and other non trivial implementation decisions.

 Agree, will do as your suggestion.

 >  - Related to the point of whether we have multiple instances, is it
 okay to make _is_shut_down and _read/write_sock class variables?  If I
 understana it correctly they will be shared by all instances, and I
 suspect it would cause undesirable effects.

 it's class variable, but it was used as self._is_shut_down, then the class
 variable will change to instance variable, so it will not cause
 undesirable effects. Also, this is what I learned from ThreadingMixIn() in
 python code.

 >  - we may want to declare ServeMixIn._serving as "private" more clearly
 by renaming it to __serving

 yes. I will do some test, since TCPServer() also has that variable with
 the same name.

 >  - Related to the documentation issue, just from the code the purpose of
 the poll_interval of serve_forever() isn't clear because it's unused.  we
 need some note in comments about this point.  Also, I'm not sure whether
 we need to bother to provide the default parameter.

 I just want to keep the interface be same, since serve_forever() in base
 class has the parameter "poll_interval", though it never be used in the
 mixin class. I should comment it in the code

 >  - There's inconsistency between comment and code:
 > {{{
 >             # block until the self.socket or self._read_sock is readable
 >             try:
 >                 r, w, e = select.select([self, self._read_sock], [], [])
 > }}}
 >   comment says self.socket whle the code uses just self.  Also, while
 not incorrect, having "self" (which is a socketserver object) and
 "_read_sock" (which is a socket object) in a list seems a bit awkward.

 I also feel awkward at begining, since it was copied from python's source
 code, after checking the documentation of select(), it says ''The first
 three arguments are sequences of ‘waitable objects’: either integers
 representing file descriptors or objects with a parameterless method named
 fileno() returning such an integer:
 ''. I don't know whether I should change self to self.fileno(), since it's
 really python style.


 >  - I'd add a sanity check for the received data from the socket pair,
 i.e., check to see if the received data is 'anydata'.  (and I'd define a
 constant variable to minimize hardcoding the magic term)

 Yes, agree. will do it.

 >
 > '''serve_mixin_test.py'''
 >  - I suggest replace 'localhost' with 127.0.0.1.  the former may require
 transaction with an external DNS server, which may block and/or fail.

 Good suggestion

 >
 > '''utils/tests/Makefile.am'''
 >  - there's a typo that makes the test fail.  I've fixed it in the branch
 (r3138)
  Thanks for your fixing. How did I make this mistake?, :)

 > '''!ChangeLog'''
 >  - is the "author" (=zhang likun) correct?

 Right, jinmei, it was did by me.

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