Hi Jensen, Thanks for your detailed comments on my review. Also, really great work on addressing each comment from a large number of reviewers.
I agree with all your comments and your plan to resolve them. Regarding this question: "How about we remove this paragraph and explain how these components work in different use cases instead?" I like your proposed approach, as this will avoid defining an interoperation requirement in an overview section (as you point out). Thanks, Jordi ________________________________ From: Jensen Zhang <jingxuan.n.zh...@gmail.com> Sent: Tuesday, May 23, 2023 14:27 To: Jordi Ros Giralt <j...@qti.qualcomm.com> Cc: draft-ietf-alto-oam-y...@ietf.org <draft-ietf-alto-oam-y...@ietf.org>; alto@ietf.org <alto@ietf.org> Subject: Re: One more review for draft draft-ietf-alto-oam-yang WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. Hi Jordi, Many thanks for your thorough review and wonderful comments. Please see my response inline. Thanks, Jensen On Sun, May 21, 2023 at 1:22 PM Jordi Ros Giralt <j...@qti.qualcomm.com<mailto:j...@qti.qualcomm.com>> wrote: Please find below comments from my latest review of the doc draft-ietf-alto-oam-yang. No major blockers from my side, mainly some comments on definitions, semantics, and nits. Many thanks. Jordi * Definitions: ------------- (1) In Section 4.4., the following terms are introduced for the first time: {'server manager', 'information resource', 'performance monitor', 'logging and fault manager', 'algorithm', 'algorithm plugin', 'data source'} Some of these terms might be intuitive, but I think it would be important to provide an explicit definition. For instance, the term 'algorithm' is quite generic, but it does have a specific meaning in the context of this specification that would be beneficial to state. Providing an example would also be very useful. Can I suggest that we add a subsection of definitions, providing a definition for each of these terms and a simple example? Thanks for the wonderful suggestion. I think it is a good idea. We will add a subsection for these terms. (2) In Section 5.2 we explain 'server-discovery-channel' but we don't explain 'client-id'. Shall we also add a sentence describing 'client-id' for completeness? Sure. We will describe 'client-id' as well. (3) In this sentence "By default, 'syslog' is the only", shall we include a reference to the syslog RFC? (E.g., RFC 5425) Good suggestion. We will add. * Semantics: -------------- (1) In the sentence "an ALTO server implementation should contain", should the 'should' be capitalized? (SHOULD) That is a good catch. But it is not a good idea to define any interoperation requirement in an overview section. How about we remove this paragraph and explain how these components work in different use cases instead? (2) The following sentence is a bit confusing in a couple of ways: "the top-level container 'alto' in the "ietf-alto" module contains a single 'alto-server' and a list of 'alto-client' that are uniquely identified" 'alto-client' is the list itself, so instead of "a list of 'alto-client'", I think it should be "a list 'alto-client'" without "of". Also, it is not clear what this part refers to: "that are uniquely identified". I think it refers to the fact that the ALTO clients are uniquely identified (via 'client-id'). But the subject in this sentence is the list 'alto-client', not the clients themselves. So I might suggest using "the top-level container 'alto' in the "ietf-alto" module contains a single 'alto-server' and a list 'alto-client'". (Without "of" and without the qualifier "that are uniquely identified".) Agree. The text will be changed to the following: OLD: ..., the top-level container 'alto' in the "ietf-alto" module contains a single 'alto-server' and a list of 'alto-client' that are uniquely identified. NEW: ..., the top-level container 'alto' in the "ietf-alto" module contains a single "alto-server" and a list "alto-client". (3) The doc states that "If 'feed-interval' is zero, the data source is expected to work in the 'on-change' mode". But wouldn't in this case 'on-change' be present and 'feed-interval' not be present? It seems redundant to have 'feed-interval' be zero and 'on-change' be present at the same time. Also in some parser implementations, it could be prone to error or add complexity If both are present. So maybe requiring that 'feed-interval' be larger than zero and delegating the case of zero to 'on-change' would keep it simpler. Good point. I agree and will produce this change. (4) The doc states that "If poll-interval is zero, the ALTO server will not fetch the data source". In this case (corresponding to proactive updates), how is data fetched? A clarification would likely help. In this case, the data source listener will not automatically fetch the data periodically. But the algorithm plugin can call the data source listener to fetch the data at runtime. We will clarify this in the corresponding paragraph. (5) This paragraph is repeated twice: "A malicious client could attempt to set a very low/ large value to this node. Setting a very low value could attack the data source. And setting a very large value would lead to maintaining stale data in the ALTO server." May I suggest: "In particular, for both 'feed-interval' and 'poll-interval', a malicious client could attempt to set a very low/large value to this node. Setting a very low value could attack the data source, while setting a very large value would lead to maintaining stale data in the ALTO server." Thanks. We will adopt the editing suggestion. * Nits: -------- s/The detailed design of the data model is illustrated by Section 5 and Section 6/The detailed design of the data model is illustrated in Section 5 and Section 6/ s/And some examples of how to extend this data model/Some examples of how to extend this data model s/the following interactions with each others/the following interactions with each other/ s/Both the server manager and information resource manager will report statistics data to performance monitor and logging and fault manager/Both the server manager and the information resource manager will report statistics data to the performance monitor and the logging and fault manager/ Instead of "The algorithm plugins will register callbacks to the corresponding ALTO information resources upon the configuration", I would suggest "upon configuration" (without 'the') or, alternatively, you can probably say "The algorithm plugins will register callbacks to the corresponding ALTO information resources at initialization time" s/A data source listener will update the preprocessed data to an optional data broker/A data source listener will send the preprocessed data to an optional data broker/ (Note: In this case, I would also omit the word 'optional', since otherwise it does not make sense to send something to a destination that does not exist.) s/Data Model for Server-level Operation and Management/Data Model for ALTO Server Operation and Management/ (note: this is to follow the same style as in heading 5.2 and 5.4) s/The 'listen' contains/'listen' contains (without 'The') s/But it does not contain/However, it does not contain s/which is sugested by/as suggested by s/the ALTO server reactively waits the data source for pushing updates. For the proactive update, the ALTO server has to proactively fetch the data source periodically/the ALTO server reactively waits for the data source to push updates. For the proactive update, the ALTO server proactively fetches the data source periodically s/To use the reactive update, two publish modes are supported/Two publish modes are available to support reactive updates s/will trigger notifications to be generated/will trigger notifications (note: could you also clarify to who are these notifications sent?) s/This basic model only includes authentication approach directly provided by the HTTP server/This basic model only includes the authentication approach provided by an HTTP server s/statistics that indicates/statistics that indicate s/More specifically, num-total-* and num-total-last-* provides server-level/More specifically, num-total-* and num-total-last-* provide server-level s/num-map-entry and num-base-obj provides measurement for number of/num-map-entry and num-base-obj provide the measurement for the number of s/num-upd-stream and num-upd-msg-* provides statistics/num-upd-stream and num-upd-msg-* provide statistics s/that can be directly measured at the ALTO server/that can be directly measured by the ALTO server (Note: I am not fully sure here, but since ALTO is not in the data plane, that 'by' is a bit more appropriate, since 'at' tens to mean "in situ") s/subset of all available protocol/subset of available protocol s/in network environments/in the network environments that they are expected to operate in (note: there are 3 occurences of this sentence in Section 8) s/their vulnerable:/their vulnerability: s/when such mode is enabled/when such a mode is enabled (note: or perhaps even better to say "when this mode is enabled" s/case in Appendix A.3 is such an example/case in Appendix A.3 provides an example s/provided by the HTTP server/provided by an HTTP server s/how a implementation-specific/how an implementation-specific s/for an IETF layer 3 unicast/for the IETF layer 3 unicast s/if the depth sets to 1, the algorithm will generate PID for every l3-node/if the depth is set to 1, the algorithm will generate a PID for every l3-node Many thanks for capturing the nits above. We will fix them in the next revision.
_______________________________________________ alto mailing list alto@ietf.org https://www.ietf.org/mailman/listinfo/alto
