Code review comment for lp:~elachuni/txamqp/heartbeat

Revision history for this message
Anthony Lenton (elachuni) wrote :

Hi Thomas,
>
> [1] There is a small conflict with current trunk.
Merged and resolved.

>
> [2] The connectionLostEvent doesn't seem to be used anywhere, it can probably
> be removed.
Right, it was being used by client code here to easily access connectionLost events, but definitely shouldn't be part of the same patch. Removed.

>
> [3] The test is a bit bad: I understand you want to test it live, but a test
> running for 8 seconds is too long. Also, setTimeout is deprecated.
I've reduced it to 3 seconds. If that's still too long we'll need to test it some other way, as you can't ask for a heartbeat interval of less than one second. And it's not calling setTimeout now.

>
> Thanks!
Thank you! :)

« Back to merge proposal