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

Improve description of max_ack_delay transport parameter #1869

Conversation

dtikhonov
Copy link
Member

The sentence it replaces is awkward:

An 8 bit unsigned integer value indicating the maximum amount of time in milliseconds by which it will delay sending of acknowledgments.

What is it -- the integer value? Can an integer value delay sending of acknowledgements?

: An 8 bit unsigned integer value indicating the maximum amount of time in
milliseconds by which it will delay sending of acknowledgments. If this
value is absent, a default of 25 milliseconds is assumed.
: An 8-bit unsigned integer value to use as the maximum delay (in milliseconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

The intent of this transport param is for the receiver to communicate it's max ack delay to the sender, so I don't think the change from "indicating" to "to use as" is quite right.

How about "An 8-bit unsigned integer value indicating the maximum amount of time in milliseconds a receiver will delay sending an acknowledgement. The receiver sends this value to the sender to set timeouts more accurately."?

I remember this was reworded a few times when I was writing it,

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's right. In this case, I how about "used as?"

The maximum delay (in milliseconds) used to send acknowledgements encoded as an 8-bit unsigned integer.

Alternatively, I can close this PR and you can improve it as you see fit, @ianswett.

milliseconds by which it will delay sending of acknowledgments. If this
value is absent, a default of 25 milliseconds is assumed.
: The maximum delay (in milliseconds) used to send acknowledgements, encoded as
an 8-bit unsigned integer. If this value is absent, a default of 25
Copy link
Member

Choose a reason for hiding this comment

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

Nor do you use a delay to send acknowledgments. Why not simply s/it/the endpoint/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this is good.

@@ -1729,8 +1729,8 @@ disable_migration (0x0009):
max_ack_delay (0x000c):

: An 8 bit unsigned integer value indicating the maximum amount of time in
milliseconds by which it will delay sending of acknowledgments. If this
value is absent, a default of 25 milliseconds is assumed.
milliseconds by which the endpoint will delay sending of acknowledgments.
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 "sending acknowledgements." reads a bit better than "sending of acknowledgments."

@martinthomson
Copy link
Member

needed a manual merge after the big reorg

martinthomson added a commit that referenced this pull request Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants