-
Notifications
You must be signed in to change notification settings - Fork 352
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
Implement asynchronous support in ODataJsonLightCollectionSerializer #2046
Implement asynchronous support in ODataJsonLightCollectionSerializer #2046
Conversation
9a755eb
to
8fe4558
Compare
8fe4558
to
1546196
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Looking at this method I have to wonder whether there's such a thing as too much async/await. Is this too fine grained to the point it might impact performance? Can/should the write async calls be clustered in a "smart" way instead of doing it almost per character/word write? Refers to: src/Microsoft.OData.Core/JsonLight/ODataJsonLightCollectionSerializer.cs:41 in 1546196. [](commit_id = 1546196, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{ | ||
var jsonLightCollectionSerializer = CreateODataJsonLightCollectionSerializer(true, container, true, writingTopLevelCollection); | ||
await func(jsonLightCollectionSerializer); | ||
await jsonLightCollectionSerializer.JsonLightOutputContext.FlushAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonLightOutputContext.FlushAsync() will flush AsyncchronousJsonWriter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuzhg The plan is to change the current implementation of FlushAsync
once implementation of asynchronous support is completed across all participating types. When we get there I'll modify it to flush AsynchronousJsonWriter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{ | ||
await this.AsynchronousODataAnnotationWriter.WriteInstanceAnnotationNameAsync(ODataAnnotationNames.ODataNextLink) | ||
.ConfigureAwait(false); | ||
await this.AsynchronousJsonWriter.WriteValueAsync(this.UriToString(collectionStart.NextPageLink)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe AsyncJsonWriter
:)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take this into consideration but it'd have to be a PR by itself since the naming was adopted in an already merged PR and it'd involve changing in quite a few places.
Issues
This pull request is a partial fulfilment of issue #2019.
Description
Implement asynchronous support in
ODataJsonLightCollectionSerializer
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.