Re: [Dime] DOIC WGLC: Editorial Comments

[Ben Campbell <[e-mail]>]

> On Dec 17, 2014, at 10:26 AM, Steve Donovan <[e-mail]> wrote:
> 
> 
> On 12/16/14, 9:02 PM, Ben Campbell wrote:
>> 

[...]

>> We keep referring to the DOIC "solution". I think "mechanism" is more appropriate. I would think of a DOIC "solution" as a deployment that leverages the DOIC "mechanism".
> SRD> I don't see an issue here, as long as we are consistent.

I think we are pretty consistent in saying "solution". I guess that seems a bit pretentious to me, but I can live with it.

>> 
>> We overuse the word Diameter in front of other things. E.g. Diameter nodes, Diameter Agents, Diameter clients, Diameter servers, etc. This entire draft is about Diameter; we don't have to keep saying it. It tends to get awkward when we use it a lot in close proximity.
> SRD> This was done based on feedback from Benoit.  We are being explicit.  If there are paragraphs where it is particularly cumbersome, please point out those paragraphs and we can try to make them less cumbersome.

I'd argue that explicit and redundant are not the same things. (The document is pretty explicit that we are talking Diameter.)  But if Benoit wants it, I won't complain further, other than to note that RFC 6733 uses the terms client, agent, and server regularly without prefixing them with Diameter.  :-)

[...]

>> 
>> -- section 2, Realm-Routed Requests: "... does not know the host that will service..."
>> 
>> I suggest: "... does not know which host will service ..."
> SRD> Ok.
>> 
>> We should also add a comment that it may be possible to apply diversion to realm routed requests.
> SRD> Where?

In the realm-routed request definition. (This was a continuation of the previous comment.)


>> 
>> -- section 3, 2nd paragraph:
>> 
>> Client, servers, and agents should not be capitalized. Should "Reporting node" and "Reacting node" be hyphenated?  Consider moving the last sentence to the start of the next paragraph.
> SRD> The capitalization of Diameter Clients, Diameter Servers and Diameter agents was Benoit's suggestion to make it explicit what is being referred to.

RFC 6733 does not generally capitalize client, agent, or server. I agree that Diameter is  a proper name, but the other terms are not. 


>  I don't see the need for hyphenation in this context.

I don't have strong feelings on that one.

>  I'm ok with moving the sentence.
>> 


>> 
>> -- section 3.2, paragraph 11: "The DCA mechanism must also allow..."
> SRD> This paragraph looks redundant.  I suggest removing it.

Isn't this the first mention of the feature vector indicating things other than algorithms? If so, I'm not sure I agree that it's redundant.

>> 
>> --> "The DCA mechanism allows..."
>> 

>> 
>> Much of this section is redundant to stuff in the parent section. I suggest reducing the parent section detail, and keeping it here. (This may be true for subsequent children of section 3.)
> SRD> Do you have specific suggestions?

Remove the 2nd to last and 3rd to last paragraph in 3.0.

>> 
>> -- 3.3, third paragraph:
>> 
>> s/ , / : /
> SRD> I don't agree.  The commas are there because it is a list.

A colon is the appropriate punctuation to set a list or enumeration off from the preceding text. This would be more obvious if the list included more than 2 items, so that it would have imbedded commas.  (e.g. "I have three things: The first thing, the second thing, and the third thing.")

>> 
>> -- 3.3, paragraph 4:
>> 
>> In this overview, I suggest text saying that HOST_REPORT indicates a host report, and just using the term "host report" the rest of the time. (Same for realm report)
> SRD> Why?

We currently aren't consistent. Sometimes we say "host report", and sometimes we say "report of type 'HOST_REPORT'". I suggest we make that consistent, and "host report" is the less cumbersome.

[...]

>> 
>> -- 3.4, para 2: " ... the definition of new report types and the definition of new scopes ..."
>> 
>> Aren't these sort of the same thing? That is, you add new scopes by extending report type?
> SRD> Can't you have a how report that applies only to sessions on that host?

By adding some new AVP that indicates sessions? I guess so.

>> 
>> -- 3.4, para 3:
>> 
>> First two sentences are a bit confusing. A new reader will likely think they contradict, without some explanation that OC-Feature-Vector is a child of OC-S-F.
> SRD> They seam clear to me.  Do you have a suggested clarification?

"A DOIC node communicates supported features by including them in the OC-Feature-Vector AVP, as a sub-AVP of OC-Supported-Features.

>> 
>> -- 3.4, para 4:
>> 
>> First sentence has already been stated more than once. No need to repeat here.
> SRD> The first sentence is there to provide context for the second sentence.

That can be done less redundantly. For example:

"Overload reports can be also extended by adding new sub-AVPs to the OC-OLR AVP, allowing ..."


>> 
>> -- 4.1.2, para 2:
>> 
>> It's probably worth reiterating that the reacting node may not be the server, and this requirement may mean inserting AVPS into answers that came from upstream.
> SRD> I don't see the need.  Do you have suggested wording?

Nevermind. I think 4.1.3 handles this.
>> 
>> -- 4.1.2, para 7: "If it selects a different algorithm, it MUST...:
>> 
>> This is also true if it needs to announce some other feature that needs to be indicated.
> SRD> This paragraph is talking specifically about algorithms and is needed because the AVP is optional.  Are you suggesting another sentence, another paragraph?  I'm guessing that the case you talk about is already covered elsewhere.

We say this in 4.1.1 on reacting node behavior. We should make the same statement for reporting node behavior. This is the definitive section on reporting node behavior. If it's repeated elsewhere, we should consider whether _that_ text is redundant.

>> 
>> -- 4.1.3, para 3: "... Agent MUST include OC-Supported-Features in request messages it receives..."
>> 
>> I suspect this should say "... requests that it _forwards_" or "_relays_".
> SRD> To differentiate between the requests that it rejects.

Or requests that it normally wouldn't relay. (e.g. watchdogs, capabilities exchange, etc.)

>  Ok, I think relays is the better word.

Ok.

>> 
>> -- 4.1.3, para 5 and 6:
>> 
>> Is the reporting node section limited to endpoints? If not, the these paragraphs effectively restate normative language about reporting node behavior from that section. Once we've said what a reporting node normatively has to do, we should just be able to say that an agent acting as a reporting node has all the responsibilities of a reporting node, and not restate the normative requirements.
> SRD> I'm ok with this but it feels like more than an editorial change to me.

I don't think it would change the meaning, just the structure. (I _did_ mention "structural" in the preface to my comments :-)  )


>> 
>> -- 4.1.3, para 7:
>> 
>> The first MAY is a consequence (the complement) of the 2nd MAY. It should at least not be 2119 language, and probably can be omitted entirely.
> SRD> Do you have suggested wording?

"If a request already has the OC-Supported-Features AVP, a Diameter agent MAY modify it to reflect the features appropriate for the transaction."

Any MAY has an an implied "MIGHT NOT". But if we think it's not clear, we can tack on "Otherwise, the agent relays the OC-Supported-Features AVP without change."

>> 
>> -- 4.1.3, para 9, last sentence:
>> 
>> Does this mean an agent can only change OC-S-F in an answer if it changed it in the request? I don't think we want that restriction, but the context and "as such," seem to scope the MAY to that situation.
> SRD> I don't read it that way.  The first sentence is describing one reason why an agent might change OC-S-F.  It doesn't say it is the only reason.

I read "as such" to make the second sentence dependent on the first. If we want to make them equal, I suggest removing the "as such".

>> 
>> -- 4.1.3, para 9: "ambiguity"
>> 
>> I'm not sure that's is the word we want here. An agent could muck things up without being ambiguous. Perhaps "inconsistency"?
> SRD> The point is that the reacting nodes and reporting nodes must unambiguously know what to do.  I think ambiguous is the right word.

I can construct behavior that is not ambiguous to upstream or downstream nodes, but still breaks normative requirements. How about " ... ensure that it does not introduce incorrect behavior for either the upstream or downstream DOIC nodes."

>> 
>> -- 4.2.1.1, para 1:
>> 
>> Do I understand correctly that the reacting node only keeps OCS for those combinations that it has actually received an OLR for? If so, that's not clear from the wording.
> SRD> Do you have suggested wording?

It depends. Does a reacting node keep OCS when it hasn't received an OLR?

If no:

"A reacting node SHOULD maintain the following OCS for each active OLR it receives per Diameter application..."

if yes:

Then I think the section needs some more thought to distinguish between what is stored for OLRs, vs when no OLR is received.

Also, I think the preface to the second bullet list seems wrong. I don't see how you could make an implementation work right without at least keeping sequence number and expiration time, at least when you have an active OLR.



>> 
>> -- 4.2.1.2, para 1:
>> 
>> Likewise, the reporting node only keeps OCS for actual overload conditions where it sends OLRs?
> SRD> Do you have suggested wording?
>> 

This seems dependent on the same question as for the last comment. But assuming it only has to keep state of actual OLRs:

"A reporting node SHOULD maintain OCS entries for each OLR it sends, per ..."

>> -- 4.2.1.3, para 2:
>> 
>> Should this combination include origin-host or origin-realm? App-Id?
> SRD> I don't understand this.

Ah, I misread the paragraph. But I think we are really talking about the algorithm, OC-OLR AVP, and other AVPs in the answer that are needed to understand the OLR. (e.g. Origin-Host, Origin-Realm, Application)

[...]

>> 
>> -- 4.2.1.3, 3rd para from end: "... update the OCS entry as being expired."
>> 
>> I suggest language like " invalid" or "terminated". I take "expired" to mean "naturally expired", which is different from the case described here. There may be subtle behavior difference. (For example, a reacting node might choose to log "expirations" differently than "explicit terminations".)
> SRD> For the context of this specification I think expired works.  Implementations can differentiate types of expired if they have a reason.  Nothing here prevents that.

Ok.

[...]

>> 
>> -- 4.2.2, 2nd to last para:
>> 
>>  I think we need something a bit stronger here. The endpoint needs to take some action that makes sense for the application. The details of that are beyond our scope, but the general need is not.
> SRD> We cannot enforce application behavior in a Diameter specification.   I don't see the need for anything more .  Do you have suggested wording?

Diameter endpoints that throttle requests [ MUST /need to ] do so according to the rules of the client application. Those rules will vary by application, and are beyond the scope of this document.


>> -- 4.2.3, para 2:
>> 
>> Doesn't this also need to match the scope for the request type? E.g. Destination realm or destination host? For example, a host report for host A would not match OCS for host B, even though both had the same report type and application id.
> SRD> I guess this matters when an agent is the reporting node.
>> 

I think it matters the same if the reporting node is an endpoint. For example, would you consider a host-routed-request with D-H: "foo" to match an host report for host "bar"?  (I'm on the fence here whether that is correct behavior, but I think the behavior should be the same for endpoints and agents.)

Actually, on reflection, how does this affect the scenario of an agent that passes through a host report and generates a realm report? Or a server that generates both?

>> -- 4.2.3, last para:
>> 
>> An example of what we have in mind could be helpful here.
> SRD> I'm open to suggested wording...
>> 

Add "For example, it might wait some period of time after overload ends before terminating the OLR, or it might send a series of OLRs indicating progressively less overload severity."

>> 
>> -- 4.3, para 4:
>> 
>> We should clarify that this is talking about mandatory to understand for DOIC, but not for the enclosing transaction.
> SRD> i'm not clear on what you are suggesting.

We need to be clear that receiving a DOIC avp with a mandatory AVP that you don't understand means you ignore the DOIC AVPs, but does not cause the Diameter transaction to fail. Or was it your intent to allow an application to say that if you get a DOIC avp that you can't process, you have to fail the Diameter transaction?

>  Do you have suggested wording?

Depends on the answer to the question above.

[...]


>> 
>> -- 5.1, para 4:
>> 
>> Convoluted paragraph. How about "How about "reporting nodes request the stateless reduction of the number of requests by an indicated percentage."
> SRD> Ok.
>> 
>> We should also clarify that this reduction is in comparison to what the reacting node otherwise would have sent, rather than what it may have previously sent.
> SRD> Isn't this already covered?  Do you have suggested wording?

I don't see where we say it in the definition of "loss". It's implied by the example logic, but I think it needs to be more explicit. How about adding the following sentence:

"This percentage reduction is in comparison to the number of messages the node otherwise would send, regardless of how many requests the node might have sent in the past."


>> 
>> -- para 6, last paragraph
>> 
>> Paragraph is off topic for section. It probably belongs in the extensibility section, or at least somewhere other than here.
> SRD> Do you mean section 6? This isn't about extensibility but about how Diameter applications integrate DOIC.

Agreed that it is probably not about extensibility. But it's not about AVP definitions, either.

>  It probably belongs in the intro or overview sections.

Agreed. I'd suggest the intro.



>> 
>> -- 6.2:
>> 
>> Why did the AVP code jump to TBD6? (Also seems out of order in the table.)
> SRD> Historical reasons.  Doesn't matter, the real values will be filled in eventually.
>> 

IANA will probably allocate them in order of TBDX numbers. I'd suggest reordering them (here and in the table) to keep child AVP definitions with their parent definition.