feat(iot-dev): Add support for Plug and Play feature over AMQP #1371
Conversation
Still TODO some multiplexing scenario pnp tests
…dk-java into timtay/pnpAmqp
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
drwill-ms and
jamdavi
as code owners
| @@ -28,7 +28,7 @@ | |||
|
|
|||
| AmqpsTelemetrySenderLinkHandler(Sender sender, AmqpsLinkStateCallback amqpsLinkStateCallback, DeviceClientConfig deviceClientConfig, String linkCorrelationId) | |||
| { | |||
| super(sender, amqpsLinkStateCallback, linkCorrelationId); | |||
| super(sender, amqpsLinkStateCallback, linkCorrelationId, deviceClientConfig.getModelId()); | |||
There was a problem hiding this comment.
Does that mean deviceClientConfig will never be null? do we need a nullcheck?
There was a problem hiding this comment.
It won't be null here, no
|
As part of PnP support we would need to add the componentName to the Message I would think. This would allow a customer to construct an Event (Telemetry) with the correct properties. Or would this be out of scope for this particular implementation? |
Isn't that transport agnostic? From the samples for PnP, I see that that twin message is constructed with the componentName, and then the message is sent just like any other. |
| assertEquals(response.getMetadata().getModelId(), E2ETestConstants.THERMOSTAT_MODEL_ID); | ||
| assertEquals(responseWithHeaders.body().getMetadata().getModelId(), E2ETestConstants.THERMOSTAT_MODEL_ID); | ||
|
|
||
| // act |
There was a problem hiding this comment.
I don't love unit tests that get overly complicated beyond a single pass at arrange/act/assert.
Should this be 2 unit tests? Can the acts and asserts be grouped together?
There was a problem hiding this comment.
This is an integration test. I can remove one of the "act" blocks here, though
|
|
||
| if (modelId != null && !modelId.isEmpty()) | ||
| { | ||
| this.amqpProperties.put(Symbol.getSymbol(PNP_MODEL_ID_KEY), modelId); |
There was a problem hiding this comment.
Do we need to worry about URL escaping the model Id for AMQP? I forget. I know we had to for MQTT.
There was a problem hiding this comment.
Good question. I'll take a look into this.
There was a problem hiding this comment.
Looks like we don't need to. We URL escaped the model Id in MQTT because it was embedded into a URL. Here in AMQP though, it is just the value in a key-value pair. I checked, and we are not URL escaping the model Id for AMQP in the .NET SDK either.
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
It's only agnostic for Twin and Methods. For events we need to have the componentName and the correct system property. Abhipsa has the AMQP implementation in the below PR. https://github.com/Azure/azure-iot-sdk-csharp/pull/1467/files |
We weren't doing this in MQTT either for some reason
I see. I pushed some changes to capture that then. I noticed that we weren't doing this in MQTT either, so I added the component name to the telemetry for both AMQP and MQTT |
| log.warn("Mqtt connection lost", throwable); | ||
|
|
||
| this.disconnect(); | ||
|
|
||
| if (this.listener != null) | ||
| { | ||
| if (ex == null) |
There was a problem hiding this comment.
ex was always null, so I removed some unnecessary blocks of code here
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
| if (message.isSecurityMessage()) | ||
| { | ||
| userProperties.put(MessageProperty.IOTHUB_SECURITY_INTERFACE_ID, MessageProperty.IOTHUB_SECURITY_INTERFACE_ID_VALUE); |
There was a problem hiding this comment.
The security message property should have been in message annotations, not in user properties
...src/main/java/com/microsoft/azure/sdk/iot/device/transport/amqps/AmqpsSenderLinkHandler.java
Outdated
Show resolved
Hide resolved
...ce-client/src/main/java/com/microsoft/azure/sdk/iot/device/transport/mqtt/MqttMessaging.java
Outdated
Show resolved
Hide resolved
…/iot/device/transport/mqtt/MqttMessaging.java Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>
…/iot/device/transport/amqps/AmqpsSenderLinkHandler.java Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
Includes some multiplexing scenario PnP tests since customers will be trying PnP out in their protocol gateways
One interesting discussion we could have here is whether we need to release this in preview first or not. You'll notice that this PR doesn't make any API changes to enable this new feature, so I'm not worried about needing to make a breaking change in our API design later. With that in mind, I feel comfortable forgoing preview as long as we are committed to supporting PnP over AMQP. I'm open to hearing otherwise, though.