[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: [OASIS Issue Tracker] (MQTT-417) WD11 Review comments by Konstantin
[ https://issues.oasis-open.org/browse/MQTT-417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=65892#comment-65892 ]
Konstantin Dotchkoff commented on MQTT-417:
-------------------------------------------
full review pass
> WD11 Review comments by Konstantin
> ----------------------------------
>
> Key: MQTT-417
> URL: https://issues.oasis-open.org/browse/MQTT-417
> Project: OASIS Message Queuing Telemetry Transport (MQTT) TC
> Issue Type: Bug
> Components: edits
> Affects Versions: 5, wd11
> Reporter: Konstantin Dotchkoff
> Assignee: Ken Borgendale
> Priority: Trivial
> Labels: editorial
>
> Review still in progress - more comments will be added to this issue
> 1. General comment about formatting of the headings:
> The formatting of the headings in Word needs to be fixed in general to show correctly in the Navigation pane. If you open the navigation pane (which I typically use), it shows many irrelevant lines as a heading.
> 2. Line 40:
> "At least once", where messages are assured to arrive but duplicates can occur.
> Insert comma after arrive -->
> "At least once", where messages are assured to arrive, but duplicates can occur.
> 3. Line 345:
> Organization of MQTT
> suggest to rename to --> Organization of the specification
> 4. Line 365 – 368:
> • Publish Application Messages that other Clients might be interested in.
> • Subscribe to request Application Messages that it is interested in receiving.
> • Unsubscribe to remove a request for Application Messages.
> • Close the Network Connection to the Server.
> Since this is a continuation of the sentence I suggest to use small caps -->
> • publish Application Messages that other Clients might be interested in.
> • subscribe to request Application Messages that it is interested in receiving.
> • unsubscribe to remove a request for Application Messages.
> • close the Network Connection to the Server.
> 5. Line 393:
> and can contain both Shared Subscriptions and Non-Shared Subscriptions.
> suggest to change to -->
> and can contain both Shared Subscriptions and regular (non-shared) Subscriptions.
> 6. Line 415:
> delete additional space character between the two sentences.
> 7. Line 420:
> use caps for Client and Server
> 8. Line 599:
> receiver (Server or Client) receives an MQTT Control Packet containing any of them it MAY close the
> add comma after them -->
> receiver (Server or Client) receives an MQTT Control Packet containing any of them, it MAY close the
> 9. Line 620:
> Thus each byte encodes 128 values and a "continuation bit".
> add comma after Thus -->
> Thus, each byte encodes 128 values and a "continuation bit".
> 10. Line 662:
> delete additional space character between the two sentences.
> 11. Line 663:
> delete additional space character between the two sentences.
> 12. Line 663:
> Where used the data consists only of the data portion of the field…
> add comma after “Where used” -->
> Where used, the data consists only of the data portion of the field
> 13. Line 675:
> secure communications options
> suggest to change to -->
> secure communication options
> 14. Line 682:
> suggest to add “, where x.x.x is the specification section number and y is statement counter within this section.” at the end of the sentence.
> 15. Line 708:
> need to update section reference number
> 16. Line 720:
> Table 2-2
> What is the reason for Bit 1 = 1 for PUBREL, SUBSCRIBE and UNSUBSCRIBE?
> 17. Line 723:
> Do we need to add something like “A length of zero indicates that there are no bytes remaining in the packet.”?
> 18. Line 759
> should it be Client-Server pairs (instead of Client Server pairs)?
> 19. Line 778:
> delete additional space character between the two sentences.
> 20. Line 781:
> Table 2-4
> Value 131 – add DISCONNECT in the packets column
> 21. Line 781:
> Table 2-4
> Value 140 – add DISCONNECT in the packets column
> 22. Line 781:
> Table 2-4
> Value 150 – add PUBACK and PUBREC in the packets column
> 23. Line 808:
> need to update section reference number
> 24. Line 836:
> delete additional space character between the two sentences.
> 25. Line 859:
> delete additional space character after “Return Code”
> 26. Line 892:
> Return Code 0x00 0x04 --> Return Code 0x00 or 0x04
> 27. Line 911:
> Cap for Client (instead client)
> 28. Line 931:
> This is true only if the Server supports retain. This statement must be changed to say the Server MUST do this only if it returns Retain available on the CONNACK.
> 29. Line 967:
> use cap for Client (instead of client)
> 30. Line 978:
> change formatting of the section number
> 31. Line 994-995:
> The Client and Server MUST store the Session after the Network Connection is closed [MQTT-3.1.2-24].
> This can be misinterpreted. To assure clarity of a normative statement, I suggest to add that they need to store the state according to the Session Expiry Interval -->
> The Client and Server MUST store the Session after the Network Connection is closed [MQTT-3.1.2-24], if the Session doesn’t expire or for the duration of the Session Expiry Interval, if specified.
> 32. Line 1062:
> delete additional space character between the two sentences
> 33. Line 1073:
> change formatting of the section number
> 34. Line 1079:
> delete additional space character between the two sentences
> 35. Line 1087:
> delete additional space character after “acknowledgements”
> 36. Line 1088:
> delete additional space character after “SHOULD NOT”
> 37. Line 1103:
> change formatting of the section number
> 38. Line 1118:
> consider replacing “this is a Protocol Error” with “it is a Protocol Error”
> 39. Line 1133:
> change formatting of the section number
> 40. Line 1140:
> should we say “send a Topic Alias in a PUBLISH packet to the Client that will cause to exceed the Topic Alias Maximum sent by the Client”?
> 41. Line 1146:
> delete additional space character between the two sentences
> 42. Line 1161:
> delete additional space character between the two sentences
> 43. Line 1171:
> or User Properties in a packet other than CONNACK or DISCONNECT, uses a DISCONNECT packet
> --> or User Properties in a packet other than CONNACK or DISCONNECT, it uses a DISCONNECT packet
> 44. Line 1182:
> delete additional space character between the two sentences
> 45. Line 1187:
> delete additional space character between the two sentences
> 46. Line 1190:
> delete additional space character between the two sentences
> 47. Line 1197:
> delete additional space character between the two sentences
> 48. Line 1198:
> delete additional space character between the two sentences
> 49. Line 1201
> Figure 3-6
> missing example properties for byte 14, 15 and 16
> 50. Line 1235
> re “When the Session Expiry Interval is long” – not only if it’s long, but if the Session expires later (doesn’t matter for how long).
> 51. Line 1251:
> delete additional space characters between the two sentences
> 52. Line 1253:
> including earlier versions of this protocol
> suggest to use this --> including other versions of the MQTT protocol
> 53. Line 1254:
> MQTTv5.0 --> MQTT v5.0
> 54. Line 1260:
> delete additional space character between the two sentences
> 55. Line 1272:
> suggest to use “DISCONNECT packet on the existing Client connection with Return Code of 0x8E”
> 56. Line 1295
> suggest to add “, unless it has set Auth method.” to the sentence.
> 57. Line 1377 --> separate issue [MQTT-422]
> Session Expiry Interval set to 0 - This should be Clean Start set to 1. The Session Expiry Interval from a previous session has an influence, but I suggest to reword by emphasizing on the previous session and saying that if the Server has a Session that hasn’t expired, then it MUST set Session Present to 1 otherwise 0.
> 58. Line 1357:
> use cap Server instead of server
> 59. Line 1362:
> use cap Server instead of server
> 60. Line 1372:
> Table 3-1
> Value 154 should be “Retain not supported” as in Table 2-4
> Description: Suggest to use ”The Server doesn’t support Retain messages.”
> 61. Line 1382:
> delete additional space character between the two sentences
> 62. Line 1388:
> change formatting of the section number
> 63. Line 1393:
> suggest to change “the number of publications” to “the number of QoS 1 and QoS 2 publications”
> 64. Line 1394:
> suggest to break the sentence into to sentences. Second sentence starts with “It does not …”
> 65. Line 1412:
> The Client might choose to send fewer than Receive Maximum messages to the Client without The Client might choose to send fewer than Receive Maximum messages to the Server without
> 66. Line 1425:
> If the Maximum QoS absent --> If the Maximum QoS is absent
> 67. Line 1439
> Use a CONNACK packet with Return Code 0x9B (QoS not supported) --> It SHOULD use a CONNACK packet with Return Code 0x9B (QoS not supported)
> 68. Line 1454:
> A value is 0 means --> A value of 0 means
> 69. Line 1463: client --> Client
> 70. Line 1465: uses --> SHOULD use
> 71. Line 1468: server --> Server
> 72. Line 1472: for the value to be sent to --> for the value to be set to
> 73. Line 1476: exceeds this limit --> exceed this limit
> 74. Line 1479: uses --> SHOULD use
> 75. Line 1491: any Session --> any other Session
> 76. Line 1515: putting this information into --> using this information in
> 77. Line 1519: additional diagnostic information --> additional information to the Client.
> 78. Line 1533: Wild Card Subscription --> Wildcard Subscription (for consistency with lines 1529 and 1530)
> 79. Line 1534: Wild Card Subscription --> Wildcard Subscription (for consistency with lines 1529 and 1530)
> 80. Line 1560: assigned by the server --> assigned by the Server
> 81. Line 1609: from Server --> from a Server
> 82. Line 1668: RETAIN flag is set to 1 --> RETAIN flag set to 1
> 83. Line 1668: the DISCONNECT Return Code --> a DISCONNECT with Return Code
> [NEW]
> 84. Line 1720-1721: difficult to understand why would the Server override the Topic Name? The Server is the one that puts it in the packet. Does it mean to modifying the Topic Name from the one that a sender has sent a message to, and then the Server changes it when it delivers to a subscriber?
> 85. Line 1724: Error! Reference source not found
> 86. Line 1734: Byte Indicates --> Byte indicates
> 87. Line 1766: seta --> sets
> 88. Line 1770: how does the sender knows whether a topic alias mapping has been set at the receiver if the message is QoS 0?
> 89. Line 1799: it would be nice if we had a mechanism to define topic alias for response topics too. In case both sides do frequent request/response, reducing the packet size would be beneficial here too. We could add another property for Response Topic Alias in the same message.
> 90. Line 1816: non-normative comment is obvious and I don’t think we need it at all. I suggest to remove it.
> 91. Line 1824: Requestor --> requestor
> 92. Line 1834: Client should also send the Correlation Data unaltered as part of the PUBLISH of the responses.
> --> Client should also send the Correlation Data unaltered as part of the PUBLISH for the Response Message
> OR
> --> Client should also send the Correlation Data unaltered as part of the Response Message
> 93. Line 1841: I suggest to remove the non-normative comment from the spec.
> 94. Line 1859: This data type --> This property
> 95. Line 1863: I suggest to remove the non-normative comment from the spec.
> 96. Line 1887: MQTT performs no --> MQTT Client or Server are not responsible to perform any
> 97. Line 1890: suggest to remove non-normative comment.
> 98. Line 1902: last table row should be converted in 2: 1 row that says Properties, formatted as the Packet Identifier row above, followed by the existing row for byte 8, but with renamed “No Properties” to “Property Length”
> 99. Line 1939: were in present in --> were present in
> 100. Line 1940: client --> Client
> 101. Line 1959: I think it should use Return Code of 0x94 Topic Alias invalid
> 102. Line 1982: this is more broader comment. For return codes, we say value of 0xXX in the text (and we use the hex value). But then in the tables we call value the decimal value and use column Hex). Would it be better to use the decimal values throughout the text of the spec?
> 103. Line 1986: Table 3-5: add return code Value 150 (0x96) “Message rate too high”
> (for example, the user property can specify the retry after interval, so that the Client can wait for that period and then continue sending messages, instead of forcing it to disconnect and perform the resource intense reconnect)
> 104. Line 1989-1990: suggest the following change (minor rewording):
> The Return Code 0x00 (Success) may be sent by using a Remaining Length of 2 --> Success may also be transmitted by using a Remaining Length of 2 and omitting the Return Code, which is equivalent to using the Return Code 0 (Success).
> 105. Line 2005: additional diagnostic information --> additional information
> 106. Line 2037: Table 3-6: add return code Value 150 (0x96) “Message rate too high”
> 107. Line 2056: additional diagnostic information --> additional information
> 108. Line 2112: additional diagnostic information --> additional information
> 109. Line 2165: additional diagnostic information --> additional information
> 110. Line 2230-2232: I suggest to swap the meaning of the 0 and 1 values – I think this would better match the option name “Retain As Published”
> 111. Line 2244: any of Reserved bits --> any of the Reserved bits
> 112. Line 2258: Figure 3-20: do we need to introduce the abbreviations RAP and NL?
> 113. Line 2278: although its maximum QoS value could be different --> although its Subscription Options could be different
> 114. Line 2280: I’m not sure I understand what “but the flow of publications MUST NOT be interrupted” means. I guess other readers might have the same issue. Maybe we can elaborate what this means.
> 115. Line 2303: The QoS of Payload Messages sent --> The QoS of messages sent
> (this is the only place in the spec where Payload Message is used).
> 116. Line 2305: server --> Server
> 117. Line 2328: server --> Server and client --> Client
> 118. Line 2334-2340: 7x (client --> Client)
> 119. Line 2337: identifier or no identifier --> Subscription Identifier or no Subscription Identifier
> 120. Line 2339: in are transmitted?
> 121. Line 2339-2341: 2x (server --> Server)
> 122. Line 2340: remade – does that mean resubscribed or maybe recreated? According to [MQTT-3.8.4-3] this would replace the current subscription, even if it has a new identifier. This means the old identifier wouldn’t be valid anymore. The paragraph is a bit confusing, so I’m not sure what we mean by remade and how a subscription might end up having 2 identifiers (an old and a new one).
> 123. Line 2391: Should we add a Variable Header non-normative example for the Packet Identifier for the SUBACK packet, similar to the other ACK packets?
> 124. Line 2406: additional diagnostic information --> additional information
> 125. Line 2418 Table 3-10
> Return Code 145 (0x91) is a in response to the entire SUBSCRIBE packet, and not for individual topic filter. We need to allow in the spec to send this one independent of the list of Return Codes. In that case I suggest we allow one or more return codes. The Server may send only 145 or a list of Return Codes containing one return code per topic filter.
> 126. Line 2418 Table 3-10: Description of Code 162: Wildcard subscription --> Wildcard subscriptions
> 127. Line 2423: formatting issue – this shouldn’t be a separate section title, but should be inside section 3.9.3. Table 3-11 is between the description for Figure 3.27 and the figure itself.
> 128. Line 2452: broken reference
> 129. Line 2454: section reference to be fixed
> 130. Line 2466: is it better to say corresponding instead of owning?
> 131. Line 2517: additional diagnostic information --> additional information
> 132. Line 2528 Table 3-12
> Return Code 145 (0x91) is a in response to the entire UNSUBSCRIBE packet, and not for individual topic filter. We need to allow in the spec to send this one independent of the list of Return Codes. In that case I suggest we allow one or more return codes. The Server may send only 145 or a list of Return Codes containing one return code per topic filter.
> 133. Line 2555: section reference needs to be fixed
> 134. Line 2600: Table 3-13: Description of code 137: cannot continue processing this Client. --> cannot continue processing requests from this Client.
> 135. Line 2600: Table 3-13: Description of code 142: has connected --> is being established
> 136. Line 2600: Table 3-13: Description of code 144: s --> is
> 137. Line 2600: Table 3-13: Description of code 150: The rate of publish --> the rate of messages
> 138. Line 2600: Table 3-13: Description of code 159: connection is closed --> connection is being closed
> 139. Line 2600: Table 3-13: is it possible to rearrange the codes, so that 158, 161 and 162 are next to each other, and 159 is next to 150 and 151?
> 140. Line 2631: is received by ?
> 141. Line 2643: additional diagnostic information --> additional information
> 142. Line 2657: Figure 3-24: should bytes 14, 15, and 16 be deleted from this example?
> 143. Line 2664: no Will message on DISCONNECT with code 0x04, contradicts the purpose of this code. Also see table 3-13 for reference, which states that Will message is supposed to be sent.
> 144. Line 2715: additional diagnostic information --> additional information
> 145. Line 2753: server Server
> 146. Line 2767: are force written --> is force written
> 147. Line 2774-2775
> The transport protocol used to carry MQTT 3.1 was TCP/IP as defined in [RFC0793]. TCP/IP can be used for MQTT 3.1.1 and MQTT 5. Following are also suitable: -->
> TCP/IP as defined in [RFC0793] can be used for MQTT 5. Following are also suitable:
> 148. Line 2785: Section 4.3 contains mixed spelling of sender / receiver and Sender / Receiver with caps. Ideally we should unify for consistency throughout the spec. My personal preference would be Client / Server and sender / receiver.
> 149. Line 2818: broken section reference
> 150. Line 2852: broken section reference
> 151. Line 2904: sequence of QoS 2 receive message handling --> sequence of QoS 2 message handling
> 152. Line 2906: double ..
> 153. Line 2920: elsewhere in this chapter? Or, elsewhere in this spec?
> 154. Line 2992-2993 unnecessary empty lines that can be deleted
> 155. Line 3037: broken section reference
> 156. Line 3084: share in the processing --> share the processing
> 157. Line 3093: Refer to section 4.7. --> Refer to section 4.7 for more information on….
> 158. Line 3200: broken section reference
> 159. Line 3213: client / server --> Client / Server
> 160. Line 3223: broken section reference
> 161. Line 3235 Requester --> Requestor
> 162. Line 3262: using Shared Subscriptions that the order --> using Shared Subscriptions the order
> 163. Line 3272: Requesters --> Requestors
> 164. Line 3275: authorization problem to use randomized Topic Name --> authorization challenge to use random Topic Name.
> 165. Line 3288: broken section reference
> 166. Line 3368: Mechanisms --> mechanisms
> 167. Line 3384: “The Server MUST set the Auth Method in the CONNACK 3384 to the Auth Method that the Client set on the CONNECT” is already covered by [MQTT-4.12.0-4].
> 168. Line 3457 and 3463 : “and a Return Code is mentioned” looks like a vague statement. Can we say something like “and Return Code is used”.
> 169. Line 3459: If --> In
> 170. Line 3470-3471: [MQTT-4.13.1-2] seems to be overlapping with [MQTT-4.13.1-1]. What is really the role of the Return Code if [MQTT-4.13.1-2] mandates MUST close the Network Connection anyways?
> 171. Line 3565: broken section reference
> 172. Line 3710: I don’t have the full context, but does this need to be updated for v5 or not?
> 173. Line 3735 and 3753: should be: - Chapter 6 - Using WebSocket as a network transport
> 174. Line 781:
> Table 2-4
> Value 145 – add PUBACK in the packets column
> 175. Line 781: Guidance which error codes are considered transient would be beneficial, but nice to have (e.g. SDKs can retry for transient conditions)
> 176. Line 1478: section 3.2.2.8: add the case of CONNECT exceeding the max packet size, which means that CONNACK can return 0x95 on CONNACK
> 177. Line 1286: If these tests succeed --> If these checks succeed
> 178. Line 1287-1288: the Server is advised not to send a CONNACK at all --> the Server may choose to not send a CONNACK at all
> 179. Line 781: Table 2-4: Add CONNACK for error code 154 (0x9A) Retain not supported
> 180. Line 1741: suggest to change MUST to SHOULD.
> 181. Line 1866: section 3.3.2.10 should include a reference to another section of the spec that explains the concept of subscription identifiers
> 182. Line 2211:
> add the following to this paragraph
> It is a protocol error to send Subscription Identifiers, if the Server doesn’t support Subscription Identifiers and has used property 41 (0x29) “Subscription Identifiers Available” on the CONNACK packet. In this case the Server MAY send a DISCONNECT packet with Return Code 0x82 (Protocol Error) as described in section 4.13 Handling errors and then close the Network Connection.
> 183. Line 2230 – 2249: need clarification about the behavior if the Server doesn’t support RETAIN
> 184. Line 2575: we should add a sentence to explicitly state what is supposed to happen with a Will message, when the Server initiates a disconnect with a DISCONNECT packet.
> 185. Line 2887 REQUIRED --> required (or use MUST for consistency)
> 186. Line 2893 - 2894:
> Historically, retransmission of MQTT Control Packets was required to overcome data loss on some older TCP networks. This might remain a concern where MQTT 5 implementations are to be deployed in such environments. --> Historically, retransmission of MQTT Control Packets on the same connection was required to overcome data loss on some older TCP networks. This might represent a challenge, if MQTT 5 implementations are to be deployed in environments where data loss on an open TCP connection is possible.
--
This message was sent by Atlassian JIRA
(v6.2.2#6258)
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]