Complete promise from the locally submitted ack #1807
Conversation
Prior to this commit any acks to a PUBLISH could fail and have their failure swallowed (hidden). I’m now thinking that it is best to propagate this particular type of failure so that the calling code is able to act on it To illustrate: 1. Publisher publishes PUBLISH with QoS 1 2. Consumer receives the PUBLISH 3. Machine is really slow 4. Consumer throws a ConsumeFailed given that 30 seconds (the local ack timeout) has passed and it hasn't received a local PUBACK 5. A PUBACK is then provided locally Step 5 will result in there being no route to the Consumer as it previously stopped at step 4. The exception reflecting the inability to route was swallowed up. It feels like an error to publish an ACK for something more than once, hence the change to propagate the error.
We increase the timeout default for slow acks given super slow machines
|
@longshorej WDYT? It appears to work for us. |
This commit restores the behaviour to that of prior to this PR, but adds in the ability for the client API to receive a notification on the acks having completed - successfully or not. This new behaviour utilises the existing completion notification mechanism for server SubAcks.
huntc
changed the title
|
Further to #1807 (comment), @longshorej and I chatted and concluded that a notification mechanism would be better than failing the stream. I've now updated the PR to use the existing SUBACK completion mechanism. I'm pretty happy with it, but will have another chat with Jason to get his feedback. |
That's all of the acks now, for consistency.
|
I'm pretty happy with this PR now. Feel free to review/merge etc. There's no change to the existing behaviour other than the increased timeout. The PR mostly introduces a new feature whereby all types of locally submitted ack can now receive an additional |
|
@2m Any chance we can get this merged? :-) |
|
Sorry for the delay. Will review today. |
2m
changed the title
|
Thanks @2m ! |
Prior to this PR any acks to a PUBLISH could fail and have their failure swallowed (hidden). I now think that it is best to signal completions so that the calling code is able to act on them; as per the existing SUBACK completion mechanism. To illustrate:
Step 5 will result in there being no route to the Consumer as it previously stopped at step 4. The local calling code has no knowledge of this and assumes that life is normal.
The test changes illustrate how this mechanism works.
I've also increased the local ack timeout to reduce the chances of above timeout. Testing on my slow Atom based machine shows that the changes work.