Opened 10 years ago

Closed 9 years ago

#22 closed task (fixed)

MsgQ review

Reported by: mgraff Owned by: hanfeng
Priority: very high Milestone: 06. 4th Incremental Release
Component: ~msgq (obsolete) Version:
Keywords: review Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

Subject says it all.

Subtickets

Change History (16)

comment:1 Changed 10 years ago by mgraff

This review is specifically for what is in branches/parkinglot/src/bin/msgq/

comment:2 Changed 10 years ago by mgraff

I believe the review is ready for round 2. I have addressed the comments provided verbally during the Jan 2010 bind 10 meeting.

comment:3 Changed 10 years ago by zhanglikun

1 For SubscriptionManager? part, the socket for each {group, instance} should check duplication.
2 For Message module, the encode and decode code isn't consistent, and some function isn't used by anyone
3 For sesssion module, some instance variable is useless like _sendbuffer, and initial value for _recvbufferlen is none, which doesn't make sense.

comment:4 Changed 10 years ago by shane

  • Owner set to mgraff
  • Status changed from new to assigned

Waiting to be merged to trunk.

comment:5 Changed 10 years ago by jreed

  • Keywords review added
  • Milestone changed from 01. Running, non-functional authoritative-only server to 02. Running, functional authoritative-only server
  • Priority changed from major to blocker

I merged to trunk. But I need someone to doublecheck any changes were made from three weeks ago until revision 736.

svn diff -r '{2010-01-14}':736 branches/parkinglot/src/bin/msgq

svn diff -r '{2010-01-14}':736 branches/parkinglot/src/lib/cc

Adjust dates above as needed -- this looks like many changes.

comment:6 Changed 10 years ago by mgraff

  • Owner changed from mgraff to jreed

I believe what is on trunk is what is best.

comment:7 Changed 10 years ago by jreed

  • Owner changed from jreed to UnAssigned

comment:8 Changed 10 years ago by shane

  • Owner changed from UnAssigned to zhanglikun

Likun, I know you reviewed this before, but we think there may have been changes.

I am also not sure if the comments from your last review were actually acted on or not - it is not clear from the ticket.

Can you please have a look at the current code for msgq and review again?

Sorry for the extra work!

comment:9 Changed 9 years ago by shane

  • Milestone changed from 02. Running, functional authoritative-only server to 05. 3rd Incremental Release

I'm pretty sure we don't have time for this release, so I'm moving this review to the 3rd incremental release this year.

comment:10 Changed 9 years ago by shane

  • Component changed from Unclassified to msgq

comment:11 Changed 9 years ago by zhanglikun

Not sure what's best time to review it? since we plan to use json, after it is down?

comment:12 Changed 9 years ago by shane

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

Okay, the JSON work is done, I think a review of msgq is probably worthwhile now.

comment:13 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to hanfeng
  • Status changed from assigned to reviewing

comment:14 follow-up: Changed 9 years ago by hanfeng

  • billable set to 0
  • Estimated Difficulty set to 0.0
  • Internal? unset
  • Owner changed from hanfeng to jelte

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?

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

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?

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]]

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]



comment:15 in reply to: ↑ 14 Changed 9 years ago by jelte

  • Owner changed from jelte to hanfeng

Replying to 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 ?

comment:16 Changed 9 years ago by hanfeng

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.