Issue 315740043: Code Review: Current path classifier

Issue 315740043: Code Review: Current path classifier (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
7 years, 6 months ago by kirill.sc

Modified:
6 years, 8 months ago

Reviewers:
lorenzo.corneo

CC:
lorenzo.corneo

Visibility:
Public.

More Reviews

Description

Hi Lorenzo, Lets do a code review before we went deep into data analysis and making some conclusions. Feel free to comment/ask about my code and I will do the same for yours. This is the tool that we use for code review. I am attaching the latest version. BR Kirill

Patch Set 1 #

Total comments: 32

Created: 7 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+1961 lines, -0 lines)			Patch
A	path_classifier/.gitignore	View	1 chunk	+1 line, -0 lines	0 comments	Download
A	path_classifier/classifier/__init__.py	View	1 chunk	+3 lines, -0 lines	0 comments	Download
A	path_classifier/classifier/pair_matcher.py	View	1 chunk	+134 lines, -0 lines	0 comments	Download
A	path_classifier/collectors/__init__.py	View	1 chunk	+3 lines, -0 lines	0 comments	Download
A	path_classifier/collectors/api.py	View	1 chunk	+106 lines, -0 lines	0 comments	Download
A	path_classifier/collectors/collector_netlink_kfilter.py	View	1 chunk	+123 lines, -0 lines	0 comments	Download
A	path_classifier/collectors/collector_proc_ktracer.py	View	1 chunk	+143 lines, -0 lines	0 comments	Download
A	path_classifier/collectors/constructor.py	View	1 chunk	+268 lines, -0 lines	0 comments	Download
A	path_classifier/common.py	View	1 chunk	+35 lines, -0 lines	0 comments	Download
A	path_classifier/config.py	View	1 chunk	+126 lines, -0 lines	0 comments	Download
A	path_classifier/dao.py	View	1 chunk	+427 lines, -0 lines	16 comments	Download
A	path_classifier/dbapi.py	View	1 chunk	+166 lines, -0 lines	0 comments	Download
A	path_classifier/intracom/__init__.py	View	1 chunk	+3 lines, -0 lines	0 comments	Download
A	path_classifier/intracom/intracom.py	View	1 chunk	+87 lines, -0 lines	0 comments	Download
A	path_classifier/models/BouncedProbe.py	View	1 chunk	+15 lines, -0 lines	0 comments	Download
A	path_classifier/models/EndNode.py	View	1 chunk	+17 lines, -0 lines	4 comments	Download
A	path_classifier/models/IcmpProbe.py	View	1 chunk	+15 lines, -0 lines	0 comments	Download
A	path_classifier/models/InitProbe.py	View	1 chunk	+14 lines, -0 lines	0 comments	Download
A	path_classifier/models/Measurement.py	View	1 chunk	+15 lines, -0 lines	6 comments	Download
A	path_classifier/models/Path.py	View	1 chunk	+45 lines, -0 lines	6 comments	Download
A	path_classifier/models/__init__.py	View	1 chunk	+3 lines, -0 lines	0 comments	Download
A	path_classifier/pc.py	View	1 chunk	+111 lines, -0 lines	0 comments	Download
A	path_classifier/tests/TestEndNode.py	View	1 chunk	+27 lines, -0 lines	0 comments	Download
A	path_classifier/tests/TestMeasurement.py	View	1 chunk	+18 lines, -0 lines	0 comments	Download
A	path_classifier/tests/TestPath.py	View	1 chunk	+53 lines, -0 lines	0 comments	Download
A	path_classifier/tests/__init__.py	View	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

kirill.sc

sample update, to make sure that email was sent out....

7 years, 6 months ago (2016-10-16 07:44:55 UTC) #1

kirill.sc

see my comments https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py File path_classifier/dao.py (right): https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcode120 path_classifier/dao.py:120: class DataManager: If this class is ...

7 years, 6 months ago (2016-10-16 13:22:28 UTC) #2

lorenzo.corneo

Hi Kirill. Here my code review. https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py File path_classifier/dao.py (right): https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcode120 path_classifier/dao.py:120: class DataManager: On ...

7 years, 6 months ago (2016-10-27 14:10:07 UTC) #3

Hi Kirill. Here my code review.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py
File path_classifier/dao.py (right):

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:120: class DataManager:
On 2016/10/16 13:22:27, kirill.sc wrote:
> If this class is not used, you should remove it from here

Acknowledged.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:183: r = [filter(lambda x: ipSrc ==
self.getEndNodeById(x[1])['ip'] and ipDst == self.getEndNodeById(x[2])['ip'],
self._tracedirections)]
On 2016/10/16 13:22:27, kirill.sc wrote:
> consider formatting this line into multiline statement. It makes it easier to
> read and you can have multiple columns of code in one screen.

this class is not longer present

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:286: self.list =  map(lambda x: EndNode(x[1], name =
x[2], id = x[0]), list(self.rdm.getEndNodes()))
On 2016/10/16 13:22:27, kirill.sc wrote:
> should it be ip=x[1] and no spaces between "=" in "name = x[2]" etc.

I usually put spaces, but I can adopt your style as new convention.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:291: self.list = self.list + [node]
On 2016/10/16 13:22:27, kirill.sc wrote:
> I believe this is slower than using append() as it creates a copy of the list
on
> the rhs

Probably, but usually I like immutability and that is a way to obtain it. For
exampl, the map() function generates all the time a new list. There may be
another thread doing something on that list...maybe this is not the case, but it
may happen..

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:313: self._trace = list(self.rdm.getTraceDirections())
On 2016/10/16 13:22:27, kirill.sc wrote:
> _traces is ambiguous name for TraceDirections

Done.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:326: def add(self, path):
On 2016/10/16 13:22:27, kirill.sc wrote:
> I have refactored this function as I mentioned before. Trace directions no
> longer have an association in Paths (for now)

Acknowledged.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:391: [ip for i, ip in enumerate(hops_chain) if (i == 0 or
i%2 == 1)])
On 2016/10/16 13:22:27, kirill.sc wrote:
> The previous code used to have set here. It was messing the order of IPs and
> removing multiple stars "*" when present. Thus making comparison between
> existing paths and newly obtained paths incorrect.

Acknowledged.

https://codereview.appspot.com/315740043/diff/1/path_classifier/dao.py#newcod...
path_classifier/dao.py:397: self.list = map(lambda x: Measurement(x[2], x[3],
idPath = x[1]), rdm.getMeasurements())
On 2016/10/16 13:22:27, kirill.sc wrote:
> The use of constants x[2] and x[3] is confusing when you dont know what they
> referring to. I suggest either to create a constants for these values or
better
> just put an assignment operator like you did for idPath=x[1], now we know [1]
is
> a idPath column

Generally, it uses the columns definition order on the DB. Nonetheless, I agree
with you. A good option may be to create a class. What do you think about it?

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/EndNod...
File path_classifier/models/EndNode.py (right):

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/EndNod...
path_classifier/models/EndNode.py:5: self.id = id
On 2016/10/16 13:22:27, kirill.sc wrote:
> Avoid using names of reserved words and built in functions id()

We could use the mongo db conventions for identifiers, namely, _id/ Would it be
better?

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/EndNod...
path_classifier/models/EndNode.py:11: return self.ip == y.ip
On 2016/10/16 13:22:27, kirill.sc wrote:
> Can 2 endnodes have the same IPs but different names? Probably the same IP can
> be assigned to a different VM. we can also add "and self.name == y.name"

Done.

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Measur...
File path_classifier/models/Measurement.py (right):

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Measur...
path_classifier/models/Measurement.py:2: def __init__(self, latency, timestamp,
pair=None, idPath=None):
On 2016/10/16 13:22:27, kirill.sc wrote:
> timestamp -> timestamp_ns

Hmm..I don't really like it...I think that information could be added in a to
string method...

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Measur...
path_classifier/models/Measurement.py:2: def __init__(self, latency, timestamp,
pair=None, idPath=None):
On 2016/10/16 13:22:27, kirill.sc wrote:
> latency -> rtt_ns

what about rtt?

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Measur...
path_classifier/models/Measurement.py:2: def __init__(self, latency, timestamp,
pair=None, idPath=None):
On 2016/10/16 13:22:28, kirill.sc wrote:
> pair -> pair_ts or pair_timestamp

Done.

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Path.py
File path_classifier/models/Path.py (right):

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Path.p...
path_classifier/models/Path.py:2: def __init__(self, hops=[], measurements=[],
id=None):
On 2016/10/16 13:22:28, kirill.sc wrote:
> rename id, see EndNode.py

_id?

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Path.p...
path_classifier/models/Path.py:17: self.measurements = self.measurements + [m]
On 2016/10/16 13:22:28, kirill.sc wrote:
> Why dont you use append here? 

same motivation as for dao...give me your feedback please

https://codereview.appspot.com/315740043/diff/1/path_classifier/models/Path.p...
path_classifier/models/Path.py:21: return hash("".join(self.hops)) ==
hash("".join(x.hops))
On 2016/10/16 13:22:28, kirill.sc wrote:
> the previous code used set and frozen set here. These data types are not order
> preserving, while we can get the same IPs in a different order or multiple
stars
> in different places. 
> 
> Moreover, we can compute hash once in the beginning, to avoid recomputing it
> every time. If we agree that hops are immutable or provide additional API to
> update_hash()

Done.

Expand All Messages | Collapse All Messages