BIND 10 #22: MsgQ review

[BIND 10 Development]

#22: MsgQ review
--------------------------+-------------------------------------------------
      Reporter:  mgraff   |        Owner:  hanfeng                    
          Type:  task     |       Status:  reviewing                  
      Priority:  blocker  |    Milestone:  06. 4th Incremental Release
     Component:  msgq     |   Resolution:                             
      Keywords:  review   |    Sensitive:  0                          
Estimatedhours:  0.0      |        Hours:                             
      Billable:  0        |   Totalhours:                             
      Internal:  0        |  
--------------------------+-------------------------------------------------
Changes (by jelte):

  * owner:  jelte => hanfeng


Comment:

 Replying to [comment:14 hanfeng]:
 > 1 in file session.py line 87, if the length of the "env" is really big
 which is greater than the limit of unsigned short, the "env" will be
 truncated, is it better to raise an exception?
 >

 actually it would raise a struct.error, however a ProtocolError("env too
 large") would be better, fixed in r3377

 > 2 in file session.py line 115 to line 117, I don't know the purpose of
 these code, it seems useless.
 >

 hmm, probably some leftover from an earlier change, removed.

 > 3 in the session, there is one queue_ to store the message sent through
 message channel, if some one only sent and no one pick up the message from
 the queue, will it be a problem for memory usage?
 >

 yes, if a program does not expect a response and hence never reads it the
 queue will grow and never be cleaned. I don't have a simple solution for
 that. Perhaps the proposed 'expect answer' change will make this at least
 less likely.

 > 4 trivial coding style:
 > 4.1 in file data.py line 34
 >   for ka in a.keys():
 >      if ka in b and a[ka] == b[ka]:
 >          to_remove.append(ka)
 > if use list comprehension in python, this part code can be written as
 follows which is cleaner
 >   duplicate_keys = [key for key in a.keys() if key in b and a[key] ==
 b[key]]
 >

 ah, nice, also in r3377

 > 4.2 in file data.py line 40, function merge, python already provide
 function update for dictionary , the difference between merge and
 update,is that merge will remove null value items. so for merge, it can be
 written as follows which maybe cleaner
 >     def merge(orig, new):
 >      if type(orig) != dict or type(new) != dict:
 >       raise DataTypeError("Not a dict in merge()")
 >      orig.update(new)
 >      remove_null_items(orig)
 >     def remove_null_items(d):
 >       for key in d.keys():
 >        if type(d[key]) == dict:
 >           remove_null_items(d[key])
 >        else if not d[key]:
 >           del d[key]
 >

 Hmm, while cleaner, this specific one won't work, we'd be modifying the
 dict while we are traversing it... making a list of keys to remove does
 though, what do you think about the version in r3378 ?

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