feat(iot-dev): Add support for Plug and Play feature over AMQP by timtay-microsoft · Pull Request #1371 · Azure/azure-iot-sdk-java

timtay-microsoft · 2021-09-27T19:05:00Z

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.

Still TODO some multiplexing scenario pnp tests

…dk-java into timtay/pnpAmqp

azabbasi · 2021-09-27T22:54:47Z

    AmqpsTelemetrySenderLinkHandler(Sender sender, AmqpsLinkStateCallback amqpsLinkStateCallback, DeviceClientConfig deviceClientConfig, String linkCorrelationId)
    {
-        super(sender, amqpsLinkStateCallback, linkCorrelationId);
+        super(sender, amqpsLinkStateCallback, linkCorrelationId, deviceClientConfig.getModelId());


Does that mean deviceClientConfig will never be null? do we need a nullcheck?

It won't be null here, no

jamdavi · 2021-09-28T17:19:50Z

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?

timtay-microsoft · 2021-09-28T17:29:42Z

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.

drwill-ms · 2021-09-28T17:59:14Z

+        assertEquals(response.getMetadata().getModelId(), E2ETestConstants.THERMOSTAT_MODEL_ID);
+        assertEquals(responseWithHeaders.body().getMetadata().getModelId(), E2ETestConstants.THERMOSTAT_MODEL_ID);
+
+        // act


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?

This is an integration test. I can remove one of the "act" blocks here, though

drwill-ms · 2021-09-28T18:00:08Z

+
+        if (modelId != null && !modelId.isEmpty())
+        {
+            this.amqpProperties.put(Symbol.getSymbol(PNP_MODEL_ID_KEY), modelId);


Do we need to worry about URL escaping the model Id for AMQP? I forget. I know we had to for MQTT.

Good question. I'll take a look into this.

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.

timtay-microsoft · 2021-09-28T18:40:54Z

/azp run

azure-pipelines · 2021-09-28T18:41:20Z

Azure Pipelines successfully started running 4 pipeline(s).

jamdavi · 2021-09-28T19:24:39Z

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.

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

timtay-microsoft · 2021-09-28T21:55:26Z

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.

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

timtay-microsoft · 2021-09-28T21:56:08Z


        if (this.listener != null)
        {
-            if (ex == null)


ex was always null, so I removed some unnecessary blocks of code here

…dk-java into timtay/pnpAmqp

timtay-microsoft · 2021-09-29T18:07:07Z

/azp run

azure-pipelines · 2021-09-29T18:07:33Z

Azure Pipelines successfully started running 4 pipeline(s).

timtay-microsoft · 2021-09-29T18:08:31Z

+        Map<Symbol, Object> messageAnnotationsMap = new HashMap<>();
        if (message.isSecurityMessage())
        {
-            userProperties.put(MessageProperty.IOTHUB_SECURITY_INTERFACE_ID, MessageProperty.IOTHUB_SECURITY_INTERFACE_ID_VALUE);


The security message property should have been in message annotations, not in user properties

…/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>

timtay-microsoft · 2021-09-30T01:41:53Z

/azp run

azure-pipelines · 2021-09-30T01:42:14Z

Azure Pipelines successfully started running 4 pipeline(s).

timtay-microsoft · 2021-09-30T21:12:55Z

/azp run

azure-pipelines · 2021-09-30T21:13:18Z

Azure Pipelines successfully started running 4 pipeline(s).

timtay-microsoft added 5 commits September 27, 2021 12:03

feat(iot-dev): Add support for Plug and Play feature over AMQP

a578e06

Still TODO some multiplexing scenario pnp tests

multiplexing scenario tests

e1412ee

Merge branch 'master' into timtay/pnpAmqp

279ef7d

samples update

5f80bc6

Merge branch 'timtay/pnpAmqp' of https://github.com/Azure/azure-iot-s…

2b9b497

…dk-java into timtay/pnpAmqp

timtay-microsoft marked this pull request as ready for review September 27, 2021 22:08

timtay-microsoft requested review from abhipsaMisra, azabbasi, drwill-ms and jamdavi as code owners September 27, 2021 22:08

Azure deleted a comment from azure-pipelines Bot Sep 27, 2021

azabbasi reviewed Sep 27, 2021

View reviewed changes

drwill-ms reviewed Sep 28, 2021

View reviewed changes

drwill-ms approved these changes Sep 28, 2021

View reviewed changes

simplify e2e test a bit

3140e60

Add component Id to telemetry

70433c5

We weren't doing this in MQTT either for some reason

timtay-microsoft requested a review from andyk-ms as a code owner September 28, 2021 21:54

timtay-microsoft commented Sep 28, 2021

View reviewed changes

timtay-microsoft added 3 commits September 29, 2021 10:47

Merge branch 'master' into timtay/pnpAmqp

6aaa0f9

fixup

950dc74

Merge branch 'timtay/pnpAmqp' of https://github.com/Azure/azure-iot-s…

b005666

…dk-java into timtay/pnpAmqp

fixup

6b03241

timtay-microsoft commented Sep 29, 2021

View reviewed changes

jamdavi reviewed Sep 29, 2021

View reviewed changes

Comment thread ...src/main/java/com/microsoft/azure/sdk/iot/device/transport/amqps/AmqpsSenderLinkHandler.java Outdated

jamdavi reviewed Sep 29, 2021

View reviewed changes

Comment thread ...ce-client/src/main/java/com/microsoft/azure/sdk/iot/device/transport/mqtt/MqttMessaging.java Outdated

timtay-microsoft and others added 2 commits September 29, 2021 14:23

Update device/iot-device-client/src/main/java/com/microsoft/azure/sdk…

d76ac8c

…/iot/device/transport/mqtt/MqttMessaging.java Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>

Update device/iot-device-client/src/main/java/com/microsoft/azure/sdk…

37a3857

…/iot/device/transport/amqps/AmqpsSenderLinkHandler.java Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>

jamdavi approved these changes Sep 30, 2021

View reviewed changes

Merge branch 'main' into timtay/pnpAmqp

e5bd1f5

timtay-microsoft merged commit 17b8232 into main Sep 30, 2021

timtay-microsoft deleted the timtay/pnpAmqp branch September 30, 2021 22:50

timtay-microsoft mentioned this pull request Oct 5, 2021

release(2021-10-05): bump package versions #1378

Merged

Conversation

timtay-microsoft commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamdavi commented Sep 28, 2021

Uh oh!

timtay-microsoft commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timtay-microsoft commented Sep 28, 2021

Uh oh!

azure-pipelines Bot commented Sep 28, 2021

Uh oh!

jamdavi commented Sep 28, 2021

Uh oh!

timtay-microsoft commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timtay-microsoft commented Sep 29, 2021

Uh oh!

azure-pipelines Bot commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timtay-microsoft commented Sep 30, 2021

Uh oh!

azure-pipelines Bot commented Sep 30, 2021

Uh oh!

timtay-microsoft commented Sep 30, 2021

Uh oh!

azure-pipelines Bot commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timtay-microsoft commented Sep 27, 2021 •

edited

Loading