Subject: Re: [apps-review] Request for review: draft-ietf-xcon-ccmp-12

From: [email-masked] (Henry S. Thompson)
Date: Mon, 09 May 2011 18:38:08 +0100

SM writes:

> The review of draft-ietf-xcon-ccmp-12 has been assigned to you and is
> due before April 28.

Should be done by the end of this week, sorry for delay.

ht
- -- 
       Henry S. Thompson, School of Informatics, University of Edinburgh
      10 Crichton Street, Edinburgh EH8 9AB, SCOTLAND -- (44) 131 650-4440
                Fax: (44) 131 651-1426, e-mail: [email-masked]
                       URL: http://www.ltg.ed.ac.uk/~ht/
 [mail from me _always_ has a .sig like this -- mail without it is forged spam]

---

From: SM <[email-masked]>
Date: Mon, 09 May 2011 12:22:00 -0700

Hi Henry,
>Should be done by the end of this week, sorry for delay.

Thanks.

Best regards,
-sm

---

From: [email-masked] (Henry S. Thompson)
Date: Sun, 15 May 2011 16:33:00 +0100

SM writes:

> The review of draft-ietf-xcon-ccmp-12 [1] has been assigned to you

Here's my draft response.  This is the first review I've done for the
IETF, and so I'm sending it only to the list in the first instance --
if some others here could briefly review my review, and let me know if
it's along the lines you would expect, takes the right tone, etc., I'd
be very grateful.  In particular, if it's really not appropriate at
this stage in the process to raise the kind of design questions I ask
below wrt 4.2 and 4.3, I'll just remove them.

ht

[1] https://datatracker.ietf.org/doc/draft-ietf-xcon-ccmp/
=================================

I have been selected as the Applications Area Review Team reviewer for
this draft (for background on apps-review, please see
http://www.apps.ietf.org/content/applications-area-review-team).

Please resolve these comments along with any other Last Call comments
you may receive. Please wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-xcon-ccmp-13

Title: Centralized Conferencing Manipulation Protocol

Reviewer: Henry S. Thompson

Review Date: 2011-05-13

Summary: [Provide a one-sentence summary of the review. Examples
include "This draft is ready for publication as an Experimental RFC",
"This draft is almost ready for publication as an Informational RFC
but has a few issues that should be fixed before publication", and
"This draft is not ready for publication as a Proposed Standard and
should be revised before publication".]

Major Issues: 

4.2 Data Management

1) The approach to detecting competing updates and their consequences
   specified here seems unnecessarily complex.  Was the alternative of
   including version numbers in _update_ messages (so that the server
   could reject any update whose target version had been superseded)
   considered and rejected?  If so, perhaps a brief explanation of why
   it was rejected might be helful at this point.

2) In a related point, the statement at the end of this section that
   "a client subscribed to . . . notifications . . . will always have
   the most up-to-date version" is clearly false, in-so-far as it
   implies that such a client is guaranteed success for any update, as
   there is clearly a race condition here.

4.3 Data Model Compliance

1) Again this approach seems unnecessarily complex -- why does the
   data model have to constrain the initiation of a conference in this
   way.  why not simply have messages which request new conference or
   new user IDs?

2) I'm also confused by the fact that _elements_ described here as
   "mandatory" are not required by the schema.  Specifically in 5.1 we
   will see that the 'confUserID' and 'confObjID' elements, which
   correspond precisely to XCON-USERID and XCON-URI which are
   described here as mandatory, appear in message type definitions as
   minOccurs="0", i.e. as optional.  If they are optional, why is the
   above gensym complexity needed?  If they are not optional, why
   doesn't the schema say so?

3) It is unusual to refer to aspects of a data model with words such
   as 'element' and 'attribute', which are better reserved for use
   with respect to _XML serializations_ of data model instances.  Ah,
   I see by looking at draft-ietf-xcon-common-data-model that the XCON
   data model is defined as an XML document.  It's undoubtedly too
   late to do anything about that, but confounding data models and XML
   serializations is usually considered to be a mistake. . .

11. XML Schema

An http URI should be provided where this schema document can be found
on its own, and an update policy for it (or, preferably, _two_ URIs,
one for exactly this schema document, and one which will be updated if
this document is revised or superseded).  (Likewise for DataModel.xsd
and rfc4575.xsd.)

12.5 CCMP Protocol Registry

Why are these registries needed?  No role is specified for them
anywhere in the body of the document. Registries are not free, and if
all the information in the registry is also in the schemas

Minor Issues: 

6.2. Alice gets detailed information about a specific blueprint

The blueprintResponse message is not schema-valid per ccmp.xsd.  On
lines 32 and 33 of the example read
                    <xcon:floor-request-handling>confirm
                      </xcon:floor-request-handling>
The problem really lies in DataModel.xsd -- whereas (correctly)
ccmp.xsd uses xs:token as the base type for enumerated types,
DataModel.xsd (in draft-ietf-xcon-common-data-model) uses xs:string,
and the string value of the above element is "confirm
                      ", which is not one of the allowed values.  The
example should be corrected, or, for preference, the schema in
draft-ietf-xcon-common-data-model should be changed to use xs:token as
the base type for join-handling-type and all other enumerated types.

A similar problem occurs in the response in 6.3
(floor-request-handling)

6.9 Alice exploits a CCMP server extension

For compatibility with the actual response given, the extension schema
document should have a target namespace, as follows:

   <?xml version="1.0" encoding="UTF-8"?>
   <xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema"
	      targetNamespace="http://example.com/ccmp-extension-schema.xsd"
	      xmlns="http://example.com/ccmp-extension-schema.xsd">

     <xs:element name="confSummary" type="conf-summary-type"/>

     <xs:complexType name="conf-summary-type">
       <xs:sequence>
	 <xs:element name="title" type="xs:string"/>
	 <xs:element name="status" type="xs:string"/>
	 <xs:element name="public" type="xs:boolean"/>
	 <xs:element name="media" type="xs:string"/>
       </xs:sequence>
     </xs:complexType>

   </xs:schema>

Or, better, the example _and_ the schema should be changed to read as
follows:

   <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
    <ccmp:ccmpResponse xmlns:info="urn:ietf:params:xml:ns:conference-info"
	   xmlns:ccmp="urn:ietf:params:xml:ns:xcon:ccmp"
	   xmlns:xcon="urn:ietf:params:xml:ns:xcon-conference-info"
	   xmlns:example="http://example.com/ccmp-extension">
      <ccmpResponse xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	  xsi:type="ccmp:ccmp-extended-response-message-type">
	  <confUserID>xcon-userid:[email-masked]</confUserID>
	  <confObjID>xcon:[email-masked]</confObjID>
	  <operation>retrieve</operation>

	  <response-code>200</response-code>
	  <response-string>success</response-string>
	  <ccmp:extendedResponse>
	     <extensionName>confSummaryRequest</extensionName>
	     <example:confSummary>
		 <title> Alice's conference </title>
		 <status> active </status>
		 <public> true </public>
		 <media> audio </media>
	     </example:confSummary>
	  </ccmp:extendedResponse>
      </ccmpResponse>
    </ccmp:ccmpResponse>

    <?xml version="1.0" encoding="UTF-8"?>
    <xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema"
	       targetNamespace="http://example.com/ccmp-extension"
	       xmlns="http://example.com/ccmp-extension">

      <xs:element name="confSummary" type="conf-summary-type"/>

      <xs:complexType name="conf-summary-type">
	<xs:sequence>
	  <xs:element name="title" type="xs:string"/>
	  <xs:element name="status" type="xs:string"/>
	  <xs:element name="public" type="xs:boolean"/>
	  <xs:element name="media" type="xs:string"/>
	</xs:sequence>
      </xs:complexType>

    </xs:schema>

Otherwise I've checked all the schemas for conformance and the
examples for schema-validity.

12.2 XML Schema Registration

Should include pointers to the RFCs which include the text of the
schema documents named as "DataModel.xsd" and "rfc4575.xsd" in the
schema docuemnt given in section 11.

12.3 Media Type Registration

It seems unlikely that the proposed extension of 'ccmpxml' will see
much use---4 characters seems to be the practical limit for
extensions.

Nits: [list editorial issues such as typographical errors, preferably
by section number]

--

---

From: SM <[email-masked]>
Date: Sun, 15 May 2011 11:15:46 -0700

Hi Henry,
>below wrt 4.2 and 4.3, I'll just remove them.

It is appropriate to raise any questions which can help improve the 
document.  I suggest leaving the questions.

The first review is usually the difficult one.  What I generally like 
to get from a review is a different perspective and you provided 
that.  You struck the right tone.  Some people take a direct 
approach, some have a diplomatic approach and some people have their own style.

>should be revised before publication".]

There are several alternatives available here.  The reviewer should 
pick one of them.  You can diverge from the template as the examples 
are there as guidance.

>Nits: [list editorial issues such as typographical errors, preferably

The reviewer does not have to look for editorial issues.  If you find 
them, list them here.  Otherwise, you can omit this section or leave it empty.

If you are comfortable with the review, you can post it after fixing 
the two points I commented on.  If you need a clarification on any of 
my comments, please do not hesitate to ask me.

Best regards,
-sm

---

From: [email-masked] (Henry S. Thompson)
Date: Mon, 16 May 2011 10:21:48 +0100

ht writes:

> be very grateful.

I got useful feedback from SM and have shipped the review.

ht
- --