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

editorialize serialization section #289

Merged
merged 14 commits into from
Dec 9, 2023
Merged

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Jan 18, 2023

  • Judicious editorial change to section 6
  • move truncation section below json-seq
  • Split optimization to guidelines and survey/speculation
  • move conversion considerations to appendix
  • fixup: promote truncation a level

@LPardue
Copy link
Member Author

LPardue commented Jan 18, 2023

To add more colour to this PR, section 6 is very large and ranges between discussing things we really want implementers to consider - like mapping to JSON or JSON-SEQ, to other information that verges on research results or speculation. optimizations. It then oddly appears to say we aren't defining these optimizations formally but that there's normative language for how their file extensions and media types are to be handled. This sort of mixed messaging is likely to come up in wider review.

So this PR is a strawman for breaking apart the section to focus on what implementers need to do in the main body. It punts the more subjective stuff to the appendix while also watering down our guidance about how they would treat extensions or media types.

One could make the argument that compression of JSON and JSON-SEQ is mature enough an optimization to make it into the main body. I could agree with that but wanted to start somewhere.

Copy link
Contributor

@rmarx rmarx left a comment

Choose a reason for hiding this comment

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

Generally, this is in good shape. The now-appendices need some editorial love to align with the (intended) tone of the rest of the text, but that might be done in another PR later.

draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rmarx rmarx left a comment

Choose a reason for hiding this comment

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

Also needs a pass to reduce line length/add wrapping at a few locations, but other than that and the nits I had, this look good to go.

can indicate that a default JSON format is being used, but that a certain
optimization of type A was applied to the file as well (see also
{{optimizations}}).
qlog schema definitions in this document, {{QLOG-QUIC}} and {{QLOG-H3}} are
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I'm the biggest fan of referring explicitly to the other 2 docs here, since this MAIN doc is supposed to be generic (except maybe for some examples).

Copy link
Member Author

Choose a reason for hiding this comment

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

Right but it's too wooly to say "other related schema" - all extensions of qlog are related to the main schema. So the question becomes are we levying recommendations on additional schema to be generic, or can they make optimization for serialization formats? The latter seems reasonable to me, it's part of tradeoff that we mention in this spec.

We have two examples that are generic but don't think we can extapolate from that. This might lean towards having a serialization section in each of the documents so that they state if they are generic or specific. I created #357 to track that.

draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
draft-ietf-quic-qlog-main-schema.md Outdated Show resolved Hide resolved
@LPardue LPardue force-pushed the editorialize-serialization-section branch from adf21f5 to c903b6a Compare December 9, 2023 20:23
@LPardue LPardue merged commit 9713e68 into main Dec 9, 2023
5 checks passed
@LPardue LPardue deleted the editorialize-serialization-section branch December 9, 2023 20:26
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.

2 participants