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

Rename Final Offset to Final Size #2285

Merged
merged 6 commits into from Jan 8, 2019

Conversation

kazuho
Copy link
Member

@kazuho kazuho commented Jan 3, 2019

Use "final size" instead of "final offset", because the latter is ambiguous. Closes #2262.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I think that this is fine, but wonder if a better definition would help. Or maybe "end offset" - the offset of the end of the stream.

"offset" was chosen specifically because that is what matters. And it allows for gaps. So a more careful definition is probably needed either way. I've a suggestion inline that uses "final size", but I think that "end offset" might still be better.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Co-Authored-By: kazuho <kazuhooku@gmail.com>
@marten-seemann
Copy link
Contributor

I don’t really understand what’s wrong with „offset“, and I’d prefer to keep that term. I find that term a lot less confusing then „size“, and then we don’t need any explanation regarding gaps and retransmitted data.

@kazuho
Copy link
Member Author

kazuho commented Jan 3, 2019

I think that "end offset" might be ambiguous the same way as "final offset" is; it could mean the offset of the final octet. Using terms like "size" (or "number of bytes") avoids that confusion. It should also be noted that we refer to the same thing as "amount of data that can be sent" in the definition of MAX_STREAM_DATA; so far I have tried to align the definition of RESET_STREAM to that.

@martinthomson's suggestion to use "flow control credit" as a term sounds good to me. Ideally we should RESET_STERAM.final_offset and MAX_STREAM_DATA.max_stream_data using the same term.

@janaiyengar
Copy link
Contributor

I don't think I understand the ambiguity with "offset". @kazuho: can you clarify what the ambiguity is? I find "offset" more accurate because, as @martinthomson notes, there can be gaps in the stream.

the stream enters the "Size Known" or "Reset Recvd" state ({{stream-states}}).
For a
stream that is reset, the final size is carried explicitly in a RESET_STREAM
frame. Otherwise, the final size is the offset plus the length of a STREAM
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth adding that for a stream that is reset, the final size might be smaller than the number of bytes sent on the stream.

Copy link
Member Author

@kazuho kazuho Jan 6, 2019

Choose a reason for hiding this comment

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

@janaiyengar Isn't that a protocol violation triggering FINAL_OFFSET_ERROR (to be renamed to FINAL_SIZE_ERROR)? Quoting from the draft: an endpoint received a RESET_STREAM frame containing a final offset that was lower than the maximum offset of data that was already received.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my suggestion was incorrect. Not sure what I was thinking.

@janaiyengar
Copy link
Contributor

As I said on the issue, I understand now and think this is a fine change.

data carried in a STREAM frame marked with a FIN flag, or 0 in the case of
incoming unidirectional streams.
The final size is the amount of flow control credit that is consumed by a
stream. Assuming that every contiguous byte on the stream was sent, the final
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
stream. Assuming that every contiguous byte on the stream was sent, the final
stream. When every contiguous byte on the stream is sent, the final

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

I'm not sure if we want to make this change, but I think this is good text overall.

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

I'm okay with this change.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Jan 8, 2019
@martinthomson martinthomson merged commit 9f04b44 into quicwg:master Jan 8, 2019
@martinthomson
Copy link
Member

I'm not convinced that size > offset, but the other changes are a definite improvement. Changing back to offset is easy enough.

The final size is the amount of flow control credit that is consumed by a
stream. Assuming that every contiguous byte on the stream was sent once, the
final size is the number of bytes sent. More generally, this is one higher
than the largest byte offset sent on the stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Too late, but there is missing a "if any".

Copy link
Member

Choose a reason for hiding this comment

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

Never too late. But I'm not following you.

Copy link
Contributor

Choose a reason for hiding this comment

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

The final size can be zero. That is not one larger than largest sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants