Mqtt5to3 adapter #528
Mqtt5to3 adapter #528
Conversation
include/aws/crt/mqtt/Mqtt5Client.h
Outdated
| @@ -324,6 +334,47 @@ namespace Aws | |||
| std::shared_ptr<Mqtt5ClientCore> m_client_core; | |||
|
|
|||
| Mqtt5ClientOperationStatistics m_operationStatistics; | |||
|
|
|||
| Mqtt5to3AdapterOptions *m_mqtt5to3AdapterOptions; | |||
There was a problem hiding this comment.
Might be clarified as I get further in but just wondering if we'll only ever have one of these adapter options per Mqtt5Client. Thinking of a scenario where we have a number of different adapters from a single Mqtt5 client. e.g. if someone makes a bunch to feed to different service clients instead of using the same one for them all. May be a non-issue.
There was a problem hiding this comment.
We only have one adapter options for each Mqtt5Client. When we create MqttConnection from the Mqtt5Client, we will pass the options in the MqttConnection, which will copy the options over.
| { | ||
| if (m_client_core == nullptr) | ||
| { | ||
| AWS_LOGF_DEBUG(AWS_LS_MQTT5_CLIENT, "Failed to create mqtt3 connection: Mqtt5 Client is invalid."); |
There was a problem hiding this comment.
Shouldn't it be the ERROR log level?
There was a problem hiding this comment.
I think it is a good idea to set the error level to ERROR for those "invalid mqtt5 client" issue here. As if the issue happened (the client core is nullptr), it usually means the Mqtt5Client is unusable, and we should expose it to user.
Since we've been using DEBUG log for all other operations, I will keep it here for now and create a separate PR to update the log level all together.
source/mqtt/Mqtt5Client.cpp
Outdated
| @@ -233,6 +247,36 @@ namespace Aws | |||
|
|
|||
| Mqtt5ClientOptions::~Mqtt5ClientOptions() {} | |||
|
|
|||
| Mqtt5to3AdapterOptions *Mqtt5ClientOptions::NewMqtt5to3AdapterOptions() const noexcept | |||
| { | |||
| Mqtt5to3AdapterOptions *adapterOptions = new Mqtt5to3AdapterOptions(); | |||
There was a problem hiding this comment.
Debatable: If m_mqtt5to3AdapterOptions is populated directly in this method, then there will be no need in allocating m_mqtt5to3AdapterOptions on the heap or even making it a pointer.
My main concern is a new call in the code: there is no issue with it, but it always enforces you to make sure that corresponding delete will be called. Maybe at least replace it with std::unique_ptr?
There was a problem hiding this comment.
In the crt we always pair *_destroy() for anything that has a *_new() that takes an allocator to make sure we clean up anything we allocate. Unsure if we have the same for something with New in cpp but I feel like I've seen New used that returns a pointer or a null with no corresponding delete or destroy. Good to stay consistent though so worth investigating.
|
Trivial: The comment in Mqtt5ClientCore.cpp currently on line 524 |
|
Looks good to me too |
Issue #, if available:
Description of changes:
Bind out the Mqtt5 to Mqtt3 adapter.
(Still need update submodules: awslabs/aws-c-mqtt#313)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.