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

readability edit of section 4.0, complementing my ongoing draft review #1014

Closed
wants to merge 4 commits into from

Conversation

benthor
Copy link

@benthor benthor commented May 10, 2022

While reviewing the draft, I found it easy to address some of the issues I saw in section 4.0 in a light edit. I also opened an accompanying issue #1015 containing the relevant review feedback.

I understand this PR as a low-key "take it or leave it" contribution, it wouldn't bother me if it gets summarily rejected

Feedback welcome: is the level of change in this PR too heavy handed or is there interest in similar feedback about other sections of the draft?

draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved
draft-ietf-taps-impl.md Outdated Show resolved Hide resolved

## Structuring Candidates as a Tree

As noted above, the considereration of multiple candidates in a gathering and racing process can be conceptually structured as a tree; this terminological convention is used throughout this document.
The considereration of multiple candidates in a gathering and racing process can be conceptually structured as a tree; this terminological convention is used throughout this document. While a tree structure is not the only way in which racing can be implemented, it does ease the illustration of how racing works.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The considereration of multiple candidates in a gathering and racing process can be conceptually structured as a tree; this terminological convention is used throughout this document. While a tree structure is not the only way in which racing can be implemented, it does ease the illustration of how racing works.
The considereration of multiple candidates in a gathering and racing process can be conceptually structured as a tree; this terminological convention is used throughout this document. While a tree structure is not the only way in which racing can be implemented, it is a useful model to use to understand and illustrate connection racing.

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't

a useful model to use to

sound slightly odd and redundant?

How about simply saying

it can serve as a useful model to illustrate connection racing

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what's slightly odd and redundant about the first version, but I guess both will work. This is for @tfpauly to answer though...


This document structures the candidates for racing as a tree as terminological convention. While a
a tree structure is not the only way in which racing can be implemented, it does ease the illustration of how racing works.
In its most basic form, the connection-establishment process might just involve identifying a single IP address (and port) to which the application wishes to connect, and starting a TCP handshake to establish a stream to that IP address, using the system's current default path (i.e., by using the current default network interface). However, each step may also differ depending on the requirements for the connection: if, instead of a known IP address, the endpoint is identified by a hostname, then there may be a choice between multiple resolved addresses; some protocols may not need any transport handshake to establish communication (such as UDP), while others may require several layers of handshakes, such as TLS over TCP; finally, there may also be multiple paths available (i.e., via additional network interfaces besides the system default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think "connection-establishment" should just be "connection establishment".

More broadly, I don't love how this makes the default seem like connecting to an IP address — "if, instead of a known IP address...". The default is going to be higher-level endpoints, and that's a core tenet of the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Perhaps replacing the start of that sentence with something like "more commonly, when the endpoint..." will solve this.

Copy link
Author

Choose a reason for hiding this comment

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

Nit: I think "connection-establishment" should just be "connection establishment".

You're the native speaker ;) I just remember one lesson on formal writing where it was said that there should always be a hyphen whenever you have compound nouns consisting of three words or more. Here, we are talking about a "connection-establishment process" and the hyphen is meant to disambiguate the term from an equally possible "connection establishment-process".

Copy link
Author

Choose a reason for hiding this comment

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

We could change

if, instead of a known IP address, the endpoint is identified by a hostname, then there may be a choice between multiple resolved addresses;

to simply state that

an endpoint is more commonly identified by a hostname which may resolve into more than one IP address;

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the benefit of this suggested change.

Copy link
Author

Choose a reason for hiding this comment

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

I think Tommy and Anna wanted to emphasize that having a name resolution step is the more common case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I'm a little unsure this specific change is really needed... and it doesn't seem to read well, but if it is needed then please consider /which may/that can/ ... and /resolve into/resolve to/

Copy link
Contributor

@tfpauly tfpauly left a comment

Choose a reason for hiding this comment

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

Thanks very much for the suggested edits! I think we can have a bit of refinement, but these are much appreciated.

@abrunstrom
Copy link
Contributor

Thanks for the suggestions @benthor! I agree on the refinements from @tfpauly, but this will improve the readability so much appreciated.

@tfpauly
Copy link
Contributor

tfpauly commented Sep 22, 2022

Many of these changes have been overtaken by other PRs, and I don't think we have consensus on the other changes. As such, I'm closing this PR.

@tfpauly tfpauly closed this Sep 22, 2022
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

5 participants