Code review comment for lp:~theiw/txstatsd/txstatsd-fix-827432

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

[1]

+ @raise twisted.internet.error.MessageLength: If the size of data
+ is too great.

Better use "If the size of data is too large" or "too big"
Also, this left me looking for a "raise twisted.internet.error.MessageLength" in the code, but I guess this comes from below?

[2]

         if self.transport is not None:
- self.transport.write(data, (self.host, self.port))
+ try:
+ return self.transport.write(data, (self.host, self.port))
+ except (OverflowError, TypeError, socket.error, socket.gaierror):
+ return None

Why are we catching all these other exceptions? Are they related to this bug or did you just look at what else could happen that is bad and worth catching? When are these other exceptions raised?

[3]

Same code as in [2], and further down in UdpStatsDClient, also in the write method, I guess we are losing track of when a DNS error happens now, since we just catch the exception and return None.

Ok, before it was a bit harsh, with a backtrace and all :) What do you think about logging something about the error instead of silently ignoring it? Or is the "None" that we return caught somewhere and somehow logged?

review: Needs Information

« Back to merge proposal