search

eclipse-hono / hono

Eclipse Hono™ Project
https://eclipse.dev/hono
Eclipse Public License 2.0
453 stars 137 forks source link

[#3585] Add ack-required command feature for Google Pub/Sub based commands #3612

Open mattkaem opened 10 months ago

mattkaem commented 10 months ago

This implements the changes proposed in #3585 for the Google Pub/Sub messaging infrastructure. Based on this implementation it should be easy to extend this also to the Kafka messaging infrastructure in the future.

sophokles73 commented 9 months ago

I understand the intent here and that it might have looked natural to extend the one-way command mechanism with the ack-request.

My understanding of why you cannot simply use the a request/response command with your devices is that your devices' firmware cannot be changed to properly respond with an ack message themselves. If that is the case then I wonder if it would not be more beneficial (and also more intuitive from a developer POV) to extend the request/response Command mechanism to allow it to be used with devices that cannot send a response on their own and let the adapters send the response on behalf of the devices instead. We already apply that pattern for negative acknowledgements so why not use it for positive acks as well? The request message could include a property ack-required which would instruct the protocol adapter to send a positive ack once it has successfully delivered the command message to the device. For MQTT, AMQP and CoAP this might be based on the transport level acknowledgements, for HTTP it might be best-effort.

WDYT?

@calohmn any thoughts?

calohmn commented 9 months ago

We already apply that pattern for negative acknowledgements so why not use it for positive acks as well? The request message could include a property ack-required which would instruct the protocol adapter to send a positive ack once it has successfully delivered the command message to the device. For MQTT, AMQP and CoAP this might be based on the transport level acknowledgements, for HTTP it might be best-effort.

@sophokles73 So, you mean having the ack-required as a possible property for both one-way and request/response commands? In case of a request/response command, that would lead to two command response messages for one command, if the device responds to that command. To me, this would look like an unusual requirement for the command client on the business application side, to be able to deal with 2 consecutive command responses for the same command. Supporting ack-required just for one-way commands on the other hand would look like a more natural extension to me, staying in the requense-response paradigm there.

EDIT: see https://github.com/eclipse-hono/hono/issues/3585#issuecomment-1960952650 for a revised view on this.

sophokles73 commented 9 months ago

@calohmn @mattkaem see my comments in the original issue #3585

sophokles73 commented 7 months ago

@mattkaem what is the status of this PR? Can it be reviewed/merged?

mattkaem commented 7 months ago

@sophokles73 There are some small changes and an updated documentation still missing. I hope I can find the time to finish this PR soon. I will let you know once it is ready for review.

sophokles73 commented 6 months ago

@mattkaem Can you provide an update on this PR's status?

mattkaem commented 6 months ago

@sophokles73 There are no news regarding this PR. Let's ignore it for the upcoming release.

mattkaem commented 5 months ago

@sophokles73 I added the things we agreed upon in our discussion in the corresponding issue (#3585). Therefor the PR is ready for review now.