BIND 10 #216: Xfrin: Implement the feature items in TODO file.

[BIND 10 Development]

#216: Xfrin: Implement the feature items in TODO file.
------------------------------+---------------------------------------------
      Reporter:  zhanglikun   |        Owner:  jinmei              
          Type:  enhancement  |       Status:  reviewing           
      Priority:  major        |    Milestone:  y2 6 month milestone
     Component:  xfrin        |   Resolution:                      
      Keywords:               |    Sensitive:  0                   
Estimatedhours:  0.0          |        Hours:  0                   
      Billable:  1            |   Totalhours:  3.0                 
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 Okay, I've completed my review for branches/trac216.

 In addition to the initial feedback, here are the review comments:

 '''xfrin.py(.in)'''

  - I suspect we now don't have to pass shutdown_flag to XfrinConnection?
 I also even think we don't need this flag within this class now that we've
 changed how to notify threads of a shutdown event.
  - I believe we should use select for connect and send while watching a
 shutdown event.  consider, e.g., the case where a shutdown event is sent
 when the primary server is dead and connect blocks.  but it's okay for me
 to leave this enhancement to another ticket (or we should rather do so)
  - why making the socket blocking explicity?  this seems to be redundant
 and unnecessary.  (also note: if and when we make connect asynchronous, we
 should rather make it non blockng)
 {{{
 +        self._socket.setblocking(1)
 }}}
  - why using connect_ex instead of normal connect?
 {{{
 +        err = self._socket.connect_ex(address)
 }}}
   this is not necessarily incorrect, but isn't consistent with other style
 of our code.  also, (according to Evan:-) relying on error codes instead
 of exceptions is not "pythonic".  note, again, we should probably use
 connect_ex if and when we do asynchronous connect.
  - in the shutdown event handling of XfrinConnection._select() (note: I
 renamed it in r2807), I think we should check the received data, not just
 consider it a shutdown due to the readability of the socket.
  - Xfrin.__init__: the initialization of _max_transfers_in doesn't make
 sense because it won't be used anywhere and will soon be overridden in
 _cc_setup().  Besides, hardcoding such a magic number in the code is not a
 good practice.
  - it's not clearly what these comments mean.  Also the lines are too
 long.
 {{{
         #the item in self._threads_zones: zone name and xfr communication
 thread.
         #The item in self._conn_sockets: a socket and xfr communication
 thread, the main thread uses
         #the socket to communicate with this xfr communication thread.
 }}}
  - the name _conn_sockets is confusing because this is a hash from sockets
 to threads.  something like _conn_sockets_to_threads would be better (and,
 in whic case _threads_zones would be renamed to _zones_to_threads, etc)
  - I'd rename 'fd' to 'socket' (or something) here:
 {{{
         for fd in self._conn_sockets.keys():
             fd.send(b"shutdown")
 }}}
   because these are not "FDs (file descriptors)".
  - this is largely a matter of taste, but _filter_hash() could be simpler:
 {{{
     def _filter_hash(self, hash):
         '''delete zone_name in self._threads_zones or a socket in
 self._conn_sockets.'''
         for key in [k for k in hash.keys()]:
             if not (hash[key]).is_alive():
                 del hash[key]
 }}}
   btw, the function description is not very clear and not really correct
 about _conn_sockets in that what's deleted is not 'socket'.
  - isn't it better to delete completed threads as soon as possible?  in
 the current implementation they are not purged until a new xfrin attempt
 occurs.
  - keys of _threads_zones should be pairs of "zone_name and rrclass".
 and, this case should be tested.
  - configuration update for "transfers_in" should be tested.

 '''xfrin_test.py'''
  - I guess MockXfrinConnection.connect_to_master() should return True.
  - please don't leave a comment line like this.
 {{{
 +        #self._max_transfers_in = 10
 }}}
   if it's not necessary, remove it; if it's necessary but is intentionally
 commented out, please also comment why.
  - I believe I've pointed this out multiple times, but it's still not
 fixed.  Please clean up the commented-out part.
 {{{
 +    def test_connect(self):
 +        #self.assertEqual(, "")
 }}}

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