Merge lp:~theiw/txstatsd/txstatsd-fix-827432 into lp:txstatsd

[Ian Wilkinson]

Proposed fix for Bug 827432.

[Sidnei da Silva (sidnei)]

What about resolving host to an ip address at startup and catching the error there instead?

[Sidnei da Silva (sidnei)]

And by that I mean converting and storing the ip address instead of the hostname.

[Ian Wilkinson (theiw)]

That assumes the host resolves to the same address when we perform the write, which for the most part it would.

However, for a long-lived service there might be potential for the host to resolve to some other address.

On Wednesday, 17 August 2011 at 19:29, Sidnei da Silva wrote:

> And by that I mean converting and storing the ip address instead of the hostname.
> --
> https://code.launchpad.net/~theiw/txstatsd/txstatsd-fix-827432/+merge/71916
> You are the owner of lp:~theiw/txstatsd/txstatsd-fix-827432.

[Sidnei da Silva (sidnei)]

Looks good. +1!

review: Approve

[Andreas Hasenack (ahasenack)]

[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

[Ian Wilkinson (theiw)]

[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.

[Andreas Hasenack (ahasenack)]

Ok, thanks for the clarifications!

+1

review: Approve

[lp:~theiw/txstatsd/txstatsd-fix-827432]

21. By Ian Wilkinson on 2011-08-17

Included review comments.