Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mattkaem
Copy link
Contributor

@mattkaem mattkaem commented Feb 2, 2024

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.

…Google Pub/Sub based commands

Signed-off-by: Matthias Kaemmer <[email protected]>
@mattkaem mattkaem force-pushed the ack-required-command-feature-for-pubsub-based-commands branch from 0d22bfb to aabafce Compare February 2, 2024 14:17
@sophokles73
Copy link
Contributor

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
Copy link
Contributor

calohmn commented Feb 7, 2024

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 #3585 (comment) for a revised view on this.

@sophokles73
Copy link
Contributor

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

@sophokles73
Copy link
Contributor

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

@mattkaem
Copy link
Contributor Author

mattkaem commented May 2, 2024

@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
Copy link
Contributor

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

@mattkaem
Copy link
Contributor Author

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

@mattkaem
Copy link
Contributor Author

mattkaem commented Jun 5, 2024

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

Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good so far. Given that this feature is not Pub/Sub specific, I think it would be appropriate to also implement and document this for the Kafka and AMQP message infrastructure as well.
I also haven't seen any test cases for the added functionality, or am I mistaken?

In contrast to a one-way command, a request/response command contains a *response-required* attribute with value `true`
and a *correlation-id* attribute, providing the identifier that is used to correlate a response message to the original
request.
In contrast to a one-way command, a request/response command contains a *response-required* or an *ack-required*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In contrast to a one-way command, a request/response command contains a *response-required* or an *ack-required*
In contrast to a one-way command, a request/response command contains _either_ a *response-required* _or_ an *ack-required*


1. **Device Response Expected:** For devices that can send a response, set `response-required` to true. This is the
default behavior for sending request/response commands. The command is sent with the *correlation-id* to the device,
which is than expected to send a corresponding command response.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
which is than expected to send a corresponding command response.
which is then expected to send a corresponding command response.

which is than expected to send a corresponding command response.

2. **Acknowledgement Only:** For devices lacking response capability, set `ack-required` to true. The command is sent
without the *correlation-id* to the device, and the protocol adapter sends an acknowledgement as a command response on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
without the *correlation-id* to the device, and the protocol adapter sends an acknowledgement as a command response on
without the *correlation-id* to the device, and the protocol adapter sends a positive acknowledgement as a command response on

without the *correlation-id* to the device, and the protocol adapter sends an acknowledgement as a command response on
behalf of the device upon receiving a transport layer confirmation (e.g., MQTT PUBACK) from the device.

**Important Note:** Setting both `response-required` and `ack-required` to true is invalid and will result in command
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should highlight this even more by formatting it as a Warning

{{% notice warning %}}
Setting both *response-required* and *ack-required* to `true` is invalid and will result in the command being rejected.
{{% /notice %}}

on *Pub/Sub*.
4. The *Business Application* consumes the command response message from an independently created subscription to
the `projects/${google_project_id}/topics/${tenant_id}.command_response` topic.
Device sends response (*response-required* set to true):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Device sends response (*response-required* set to true):
Device sends response (*response-required* set to `true`):

`projects/${google_project_id}/topics/${tenant_id}.command` topic on *Pub/Sub*.
2. Hono consumes the message from *Pub/Sub* and forwards it to the device as a one-way command, provided that the
target device is connected and is accepting commands.
3. The device acknowledges the command on the transport layer. Hono writes an acknowledgement message to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
3. The device acknowledges the command on the transport layer. Hono writes an acknowledgement message to
3. The device acknowledges reception of the command on the transport layer. Hono writes an acknowledgement message to

@@ -121,18 +145,29 @@ command message.
|:---------------------------------------------|:---------:|:----------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| *correlation-id* | yes | *string* | The identifier used to correlate a response message to the original request. It is used as the *correlation-id* attribute in the response. |
| *device_id* | yes | *string* | The identifier of the device that the command is targeted at. |
| *response-required* | yes | *boolean* | MUST be set with a value of `true`, meaning that the device is required to send a response for the command. |
| *response-required* | no | *boolean* | Either *response-required* or *ack-required* MUST exclusively be set with a value of `true` (XOR). *response-required* set to `true` means that the device is required to send a response for the command. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| *response-required* | no | *boolean* | Either *response-required* or *ack-required* MUST exclusively be set with a value of `true` (XOR). *response-required* set to `true` means that the device is required to send a response for the command. |
| *response-required* | no | *boolean* | `true` if the device is required to send a response for the command. *response-required* and *ack-required* MUST NOT both be set to `true` at the same time. |

@@ -121,18 +145,29 @@ command message.
|:---------------------------------------------|:---------:|:----------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| *correlation-id* | yes | *string* | The identifier used to correlate a response message to the original request. It is used as the *correlation-id* attribute in the response. |
| *device_id* | yes | *string* | The identifier of the device that the command is targeted at. |
| *response-required* | yes | *boolean* | MUST be set with a value of `true`, meaning that the device is required to send a response for the command. |
| *response-required* | no | *boolean* | Either *response-required* or *ack-required* MUST exclusively be set with a value of `true` (XOR). *response-required* set to `true` means that the device is required to send a response for the command. |
| *ack-required* | no | *boolean* | Either *response-required* or *ack-required* MUST exclusively be set with a value of `true` (XOR). *ack-required* set to `true` means that the protocol adapter will try to send an acknowledgement response for the command on behalf of the device in case the command was successfully received by the device. This mechanism has some limitations which are described in the info field below. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| *ack-required* | no | *boolean* | Either *response-required* or *ack-required* MUST exclusively be set with a value of `true` (XOR). *ack-required* set to `true` means that the protocol adapter will try to send an acknowledgement response for the command on behalf of the device in case the command was successfully received by the device. This mechanism has some limitations which are described in the info field below. |
| *ack-required* | no | *boolean* | `true` if the protocol adapter should send an acknowledgement response for the command on behalf of the device in case the command was successfully received by the device. This mechanism has some limitations which are described in the info field below. *response-required* and *ack-required* MUST NOT both be set to `true` at the same time. |


An application can determine the overall outcome of the operation by means of the response to the command. The response
is either sent back by the device, or in case the command could not be successfully forwarded to the device, an error
command response message is sent by the Hono protocol adapter or Command Router component.
is either sent back by the device (response-required) or the adapter (ack-required), or in case the command could not be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is either sent back by the device (response-required) or the adapter (ack-required), or in case the command could not be
is either sent back by the device (*response-required*) or the adapter (*ack-required*), or in case the command could not be

@@ -169,6 +204,9 @@ The semantics of the individual codes are specific to the device and command. Fo

**Response Message sent from Hono Component**

If the command response is an acknowledgement of a command by the Hono protocol adapter, it has a status code
of *202* and the *content-type* attribute is set to *application/vnd.eclipse-hono-delivery-success-notification+json*.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FMPOV this type of message should be added to the Event API's list of well known events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants