Mqtt5 enums #235
Mqtt5 enums #235
Conversation
There was a problem hiding this comment.
The data classes looks good to me. I left some comments worthy discussions.
While it is a huge file, would we like to put them in separate files? For example, we could put data types in Mqtt5Types.swift, and packets in Mqtt5Packets.swift. I'm not sure what is the best practice in swift yet, but it is worthy thinking about.
I realized we probably would need other data structs for the complete callbacks. As an example, we have PublishCompletionData in python for Publish() results. I also wanna clarify that we might add new data classes in the future for those cases.
Source/AwsCommonRuntimeKit/io/retryer/ExponentialBackoffJitterMode.swift
Outdated
Show resolved
Hide resolved
|
|
||
| // TODO WebSocket implementation | ||
| /// This callback allows a custom transformation of the HTTP request that acts as the websocket handshake. Websockets will be used if this is set to a valid transformation callback. To use websockets but not perform a transformation, just set this as a trivial completion callback. If None, the connection will be made with direct MQTT. | ||
| // var websocketHandshakeTransform: Callable[[WebsocketHandshakeTransformArgs], None] = None |
There was a problem hiding this comment.
Is there any concern about websocketHandshakeTransform?
There was a problem hiding this comment.
I haven't dug into it yet. I was thinking about digging into it once we have normal connections succeeding. What concerns do you mean?
| /// The payload of the publish message in a byte buffer format | ||
| var payload: Data? |
There was a problem hiding this comment.
Debatable: "Data?" leads to two different ways of having empty data: nil vs Data(). Perhaps just default this to Data() to indicate that it is not required, instead of making it nullable if it makes sense from API perspective.
There was a problem hiding this comment.
Same with String? and others.
There was a problem hiding this comment.
I checked on implementation on what is okay to have as empty/default vs. nil and the optionals are that way because they are truly either there or not there on a received packet. Customers should be using a nil check before attempting to access them because depending on whether it exists or not can make a difference in how they handle those packets.
In the direction of customer created packets/options/etc, they can set them but would not need to access them afterwards and we will check and apply defaults after they are bound to native and processed by the client there. We should not be applying native level defaults to swift level properties.
| /// The time interval to wait after sending a CONNECT request for a CONNACK to arrive. If one does not arrive, the connection will be shut down. | ||
| var connackTimeoutMs: UInt32? |
There was a problem hiding this comment.
Same, probably easier to just default this to 0 instead of nullable?
There was a problem hiding this comment.
Same as above. Once the customer sets (or doesn't set) these values at creation of the object, they will not need to access them again so keeping them nil works for binding them as optional into the native client. The native client will process and apply any defaults as necessary at that point.
| var keepAliveIntervalSec: UInt16? | ||
|
|
||
| /// A unique string identifying the client to the server. Used to restore session state between connections. If left empty, the broker will auto-assign a unique client id. When reconnecting, the mqtt5 client will always use the auto-assigned client id. | ||
| var clientId: String? |
There was a problem hiding this comment.
I remember IoT had a bug with auto-assigned client id. Should we force assign a id for now to work around it?
There was a problem hiding this comment.
I vaguely remember something like that but I also think if you don't provide a clientId in MQTT5, the broker assigns one to you and you get it back in the negotiated settings. Maybe it was an MQTT3 specific issue.
First pass of Mqtt5 enums, classes, and callbacks.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.