Re: [SAM] [irsg] IRSG review for draft-irtf-samrg-sam-baseline-protocol-02

[Dr Mario Kolberg <[e-mail]>]

Thanks Michael! We will work on addressing your comments and issue a 
revision.

Mario

On 06/03/2013 13:35, Michael Welzl wrote:
> Hi,
>
> Here's my review. This is an outsider review if there ever was one, as I know nothing about RELOAD...
> so I just had to try to make sense of the document as good as I could - I hope my comments are useful anyway!
>
> Here it goes...
>
>
> General higher-level comments:
>
> I found parts of the document very hard to read, sometimes wondered if this is really necessary.
>
> In particular, P2PCast appears to be a rather complex algorithm which is "sort of" described here... I doubt that the description in the document will help most readers to really fully understand P2PCast, and I  wonder, is it necessary for this doc to try to really explain the algorithm (when, in doing so, it can really only go half-way anyway)? e.g., wouldn't it suffice to just keep the "Overview" (section 9.1) but then point to [P2PCAST] for further details, and just list the necessary facts? e.g., the JOIN procedure - do we have to know all these details here, isn't it enough to e.g. list the reorganisation messages by name and say that they're used in accordance with [P2PCAST]?
>
> Another source of confusion: the Scribe algorithm description has some pseudocode that I wasn't able to parse (maybe it refers to RELOAD things?) Not all functions there seem to clearly relate to messages that were described earlier. Even more confusing, the P2PCast algorithm description doesn't have these pseudocode snippets, so the whole thing appears inconsistent. Should it be everywhere? Should it be removed? If it stays, some explanations are needed. It can't be up to the reader to guess what e.g. "invokeMessageHandler" does, right?? (section 8.7). In this particular section, I would also expect the text and/or pseudocode to somehow draw a relationship to "push" (listed in fig. 3), but that's not there... so what is this?
>
> Structure: even if the two algorithms are only a part of the whole thing you define, the text going with them feels a bit like "we've set the stage, now we apply it - the messages you heard about before are used like this & that with this & that algorithm". That's fine! But it also gives the reader the feeling of that being "part 2", i.e. I think it should be at the end, if that makes sense.
>
> I also think that it would be better to move the examples (section 12) to where you introduce the messages, the flow of which they illustrate. First I have to imagine what an "INVITE" message flow could look like, then I get complicated explanations of how INVITE is applied in an algorithm that I can't fully understand from this text anyway, and THEN I get a nice example illustrating an INVITE message flow... that's not very helpful for the reader I think.  e.g., when I read 7.2.3, I wondered why the "peer_id" of the source peer is even needed in the struct. By looking at the example, this would have become clear to me.
>
> Speaking of the examples, what's the point of showing more peers than you actually use in the example? Okay, I can understand that perhaps you wanted to have the same number for all of them, but then you could still remove P3 because it seems that you never use P3 for anything in any of the examples.
>
>
> Nits:
>
> IANA Considerations: are you sure that there is nothing? e.g., how would a new algorithm code be assigned?   (section 11.4)
> - btw, why is the section called "Algorithm Codes" but then the text talks about "Algorithm Types" ?
>
> Some references (CASTRO2003, P2PCAST) need a space (I mean, " ") after their first appearance.
>
> p10: "Here is a simplistic algorithm for forming..." => but what follows is not an algorithm description?
>
> p 11 end of section 6: this ends with a colon, but no text following it... loks as if there is something missing here.
>
> p11, sec 7.1, par 1: "*makes* the SAM framework *is*" seems to be broken English
>
> 7.2.1 par 2 line 2: "other Peers" => "other peers"
>
> p13, session_key: "a well-known string **that**, when hashed ..."
>
> 7.2.3 par 2 remove "then" in "...(see below) is then sent."
>
> p14 par below "JoinDecline" in the list of messages: "...until either *a* retry limit is reached ..."
>
> p15 7.2.6 par 1: "before the expiration of the JoinAccept" => this stands out here, as a strange requirement to impose on the peer receiving JoinAccept. I think the only thing you can put here is a note that this message would have to be received by the other end before a timer expires, and say that the specifics of this timer are up to the algorithm (hm, are they? Or is this a RELOAD thing?).
>
> p 16 7.2.8 par 1: "...receiving a JoinAccept message which *it* does not wish..."
>
> sec 7.2.15: "that it has received" seems broken English here?
>
> sec 7.2.18: priority: you only say that a node may ignore this field, but not what it should otherwise do - is this up to the algorithm, in which way are priorities handled?
>
> p 22 sec 7.2.19 last par: "nodes which were forwarded the push message" - not sure this is correct English
>
> caption of figure 3: Scibe => Scribe
>
> sec 84 ", which will travel up the multicast tree and will stop at a node which has still children remaining" => ", from where it will travel up the multicast tree and stop at a node that still has children remaining"
>
> sec 9.1 par1: "independent on" => "independent of"
>
> sec 9.5 par 1: "Distregarding" => "Disregarding"
> same par: "a bubble is created" => no clue what that's supposed to mean
>
> sec 11.1 algorithm: paragraph - " ... multicast algorithm*s* can co-exist..."
>
> sec 12.2 par 1: end with a "."
>
> same in sec 12.4
>
> and sentence 2 of 13.1
>
>
> Have fun!!
> Michael
>
>
>
> On 21. feb. 2013, at 16:31, Thomas C. Schmidt wrote:
>
>> Great Michael,
>>
>> many thanks!
>>
>> Cheers,
>>
>> Thomas
>>
>> On 21.02.2013 15:58, Michael Welzl wrote:
>>> I'm no expert on any of this, but stand up and volunteer nevertheless...
>>>
>>> Michael
>>>
>>>
>>> On 21. feb. 2013, at 12:07, Thomas C. Schmidt wrote:
>>>
>>>> Hi all,
>>>>
>>>> SAMRG has completed work on
>>>>
>>>>             Application Layer Multicast Extensions to RELOAD
>>>>                draft-irtf-samrg-sam-baseline-protocol-02
>>>>
>>>> http://tools.ietf.org/html/draft-irtf-samrg-sam-baseline-protocol-02
>>>>
>>>> http://trac.tools.ietf.org/group/irtf/trac/ticket/54
>>>>
>>>> We are soliciting IRSG reviews now. Please let me know if you can review this draft.
>>>>
>>>> Thanks,
>>>>
>>>> Thomas
>>>>
>>>> --
>>>>
>>>> Prof. Dr. Thomas C. Schmidt
>>>> ° Hamburg University of Applied Sciences                   Berliner Tor 7 °
>>>> ° Dept. Informatik, Internet Technologies Group    20099 Hamburg, Germany °
>>>> ° http://www.haw-hamburg.de/inet                   Fon: [phone] °
>>>> ° http://www.informatik.haw-hamburg.de/~schmidt    Fax: [phone] °
>> -- 
>>
>> Prof. Dr. Thomas C. Schmidt
>> ° Hamburg University of Applied Sciences                   Berliner Tor 7 °
>> ° Dept. Informatik, Internet Technologies Group    20099 Hamburg, Germany °
>> ° http://www.haw-hamburg.de/inet                   Fon: [phone] °
>> ° http://www.informatik.haw-hamburg.de/~schmidt    Fax: [phone] °
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.2899 / Virus Database: 2641/6150 - Release Date: 03/05/13
>
>
>


-- 
The University of Stirling is ranked in the top 50 in the world in The Times Higher Education 100 Under 50 table, which ranks the world's best 100 universities under 50 years old.
The University of Stirling is a charity registered in Scotland, 
 number SC 011159.