Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Transport Property Profiles #328

Merged
merged 6 commits into from Jul 24, 2019
Merged

Add Transport Property Profiles #328

merged 6 commits into from Jul 24, 2019

Conversation

philsbln
Copy link
Contributor

@philsbln philsbln commented May 21, 2019

This addresses Issue #325

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented May 21, 2019

So, some comments:

  • First I suggest that we explicitly refer to these but the service they provide, not the protocol they resemble. Hence:

• The final one would be "datagram service" and UDP is an example of such.

• I would also suggest it is useful to say that the application-supplied properties can now override these defaults, hence in the Datagram case, the property permitting/requiring partial coverage could result in UDP-Lite being selected instead of UDP to supply this service.

• Then, I have a question about the specific UDP/Datagram service. This currently says:

Property Value
reliability avoid
preserve-order avoid
congestion-control avoid
preserve-msg-boundaries require
idempotent true

To me some of these are not really requirements, but to me seem like side affects. The requirement could be to need low latency, and idempotence, preserve-msg-boundaries, etc
but I wouldn't care if the service also happened to provide the following, as long as those requirements (or whatever we agree upon) were prioritised: reliability, preserve-order , congestion-control. (Of course UDP would not offer these, and that would also be fine).

Thoughts?

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented May 21, 2019

I agree with the first point and updated the PR accordingly.

For the second one, I also agree that avoiding reliability, order preservation, and congestion-control are side effects of latency or other optimisations and do not really belong into the profile. Therefore, I changed "avoid" to "ignore".
I am a bit hesitant about speaking about UDP lite here given your first point, tried to incorporate it as a hint in a slightly different way.

After these changes, I would like to raise the question whether we should mention "udp like" or "tcp like" for the impatient searching the document.

gorryfair
Copy link
Contributor

@gorryfair gorryfair commented on e3b04fe May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, thanks for checking my inputs. Will comment on the integrated text, unless there is more debate

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented May 21, 2019

Something to help start thinking: we could say something like this datagram service resembles the service offered by the socket API that binds to the UDP transport?

@gorryfair gorryfair closed this May 21, 2019
@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented May 21, 2019

Something to help start thinking: we could say something like this datagram service resembles the service offered by the socket API that binds to the UDP transport?

@philsbln philsbln reopened this May 21, 2019
@mwelzl
Copy link
Contributor

@mwelzl mwelzl commented May 24, 2019

I checked this now and like it. I also read the dialogue and agree with all points that were raised and how they were answered.

Regarding @gorryfair 's suggestion to say: "we could say something like this datagram service resembles the service offered by the socket API that binds to the UDP transport?" - actually, when reading the update first before looking at the comments, I thought "why doesn't this say: 'an example of a protocol that provides this service is UDP' ?". I think this is almost the same thing, but I like my own phrasing better :) the whole "socket API that binds to" thing seems a bit clumsy to me and I don't think we need it.

@philsbln philsbln changed the title Proposal for Transport Property Profiles Add Transport Property Profiles May 24, 2019
Copy link
Contributor

@britram britram left a comment

Thanks, this is useful! However, I think it would be better self-contained in the appendix.

@@ -655,9 +655,13 @@ Selection, and Connection Properties, as well as defaults for Message Properties
They are collected into a TransportProperties object to be passed into a Preconnection object:

~~~
TransportProperties := NewTransportProperties()
TransportProperties := NewTransportProperties(profile?)
Copy link
Contributor

@britram britram Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very useful in the appendix, but adding language here in the normative section might lead to some of the confusion that putting this in the appendixi is meant to avoid. Consider leaving this section unchanged, and make the modification to NewTransportProperties() in the appendix where properties are introduced.

Copy link
Contributor Author

@philsbln philsbln Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm… I think it is a much worse idea to modify function signatures in the appendix

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented Jul 4, 2019

I liked the split between saying "there are profiles" in the normative part and defining examples for these in the appendix. If this is a Problem, we should rather move the profiles into the property section. I have a bad feeling about modifying function signatures defined in the main documents in the appendix.

Any other opinions?
Leave un-merged and discuss in Montreal?

@mirjak
Copy link
Contributor

@mirjak mirjak commented Jul 8, 2019

I actually think this is useful but more an implementation question. I don't think this need to a part of the base API, so having this in the appendix or the implementation draft seems more suitable for me.

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented Jul 8, 2019

@mirjak What about the Parameter to pass the profile – is re-defining it in the appendix the right way?

@mirjak
Copy link
Contributor

@mirjak mirjak commented Jul 8, 2019

The API we define is anyway mostly a recommendation and will not be normative; so you will always she slight variations in the implementation. Proposing an optional parameter (in the appendix or implementation draft) seems okay.

Copy link
Contributor

@tfpauly tfpauly left a comment

I believe that profiles are conveniences that are layers implementations can build on top of the TAPS interface, but they do not need to belong in the API definition, and I think they're unnecessary to add here. At the most, we can say that implementations MAY make conveniences that are implementation -specific.

@britram britram merged commit 6d91305 into master Jul 24, 2019
1 check passed
@philsbln philsbln deleted the philsbln/propery-profiles branch Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants