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:  5.0                 
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by shentingting):

  * owner:  shentingting => jinmei


Comment:

 Replying to [comment:37 jinmei]:
 Replying to [comment:36 jinmei]:
 > Replying to [comment:35 shentingting]:
 >
 > > > > ponit3: I changed select function, now it can select the socket
 which is writable or readable.
 > > > >
 > > > Quick check: why do we need to check writability?
 > > > (intentionally leaving the owner being me)
 > > because select is asyncronous, so I set the socket non-blocking. The
 connect function is non-blocking, it will return immediately when we call
 it, this will generate EINPROCESS exception, we will ignore this and use
 select to tell us whether it is really readable or writable.
 >
 > I've given a more complete review to the latest change, and, I regret to
 say this but I'm not so happy about it.
 >
 > But before discussing that point, I'd like to check this point: are you
 sure you addressed (including rejecting some possibly) all points?  From a
 quick check this one is still open:
 >
 >  - I guess !MockXfrinConnection.connect_to_master() should return True.
 >
 year, I miss this. I have already fixed this.
 > Admittedly this is a minor point, but since I've not seen any response
 on the comment or code change, I cannot be sure if you even did any
 decision about this, and this one also makes me wonder there may be other
 remaining issues.
 >
 > Now, about the latest change.  I guess you tried to address this
 comment:
 >
 >  - 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)
 >
 > I appreciate the effort, but the code doesn't solve the real issue:
 "while watching a shutdown event".  This code essentially does blocking
 connect/write by doing select() only for the writability followed by the
 corresponding write operation.  Also, scattering select() calls over the
 source code decreases readability.
 >
 > As I commented before, I suspect this will be a sufficiently large
 changeset and we should defer it to a new ticket, especially because we've
 been spending quite a long time for this ticket already.  May I
 respectfully suggest we cancel the changes for non-blocking connect/send?
 >
 Okay, I cancel the latest change and leave it to the next ticket.
 > Some other comments on specific parts of the code:
 >
 >  - why do we need to cover EALREADY?
 > {{{
 > +            if why.args[0] in (EINPROGRESS, EALREADY, EWOULDBLOCK):
 > +                return
 > }}}
 I did not consider this. yes, I can not happen in XFRIN function. I
 deleted the EALREADY error code.
 >   As far as I can see we shouldn't see this error in our scenario.
 >  - why was the "It's extracted..." comment removed?
 > {{{
 >          This method is a trivial asynchronous I/O routine using select.
 > -        It's extracted from _get_request_response so that we can test
 the
 > -        rest of the code without involving actual communication with a
 remote
 > -        server.'''
 > +        It checks which socket is writable or readable.'''
 > }}}
 >   This part still applies with the latest change, and IMO this type of
 "why we do this" is one of the most important comments.  "What we do" type
 of comment like "It checks which..." may also help, but (IMO) is less
 important than "why" because, especially in such a trivial code, "what" is
 often obvious (for that matter, "writable or readable" is not really
 correct, because, at least according to the method interface, we also
 catch error events).
 >
 okay, I canceled the change.
 > p.s. you don't have to feel sorry for anything.  what I've been asking
 is basically "my rule", rather than agreed review protocol of the team.
 You may also have your own rule about how to handle review comments, and
 that's probably just different from mine.  I've been asking what I've
 asked based on my rule, but you may think it's not reasonable.  If so,
 please feel free to make an objection.  We can then discuss where we can
 make a compromise.
 First, thanks again. From your rules I learned a lot. I am not careful
 enough and can not consider all aspect, which wastes a lot you time. Now,
 I will do this more carefully.

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