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