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

[1] I'll change to "too large". The MessageLengthError (I'll correct the docstring) is raised by Twisted in its udp.Port write().

[2] The exceptions we catch relate to resolving the address. I think it reasonable to silently consume these. The returned None will allow the caller to log some warning.

[3] Similar to [2]. None can be the indication for the caller to log a warning.

I think your original bug caught the spirit of the expected txstatsd client behaviour: send-and-forget...

On Wednesday, 17 August 2011 at 21:11, Andreas Hasenack wrote:

> Review: Needs Information
> [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?
>
>
> --
> https://code.launchpad.net/~theiw/txstatsd/txstatsd-fix-827432/+merge/71916
> You are the owner of lp:~theiw/txstatsd/txstatsd-fix-827432.