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

RFC: incremental delivery with deduplication + concurrent execution #1026

Closed
wants to merge 63 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 21, 2023

These spec edits should correspond to the working implementation of incremental delivery with deduplication currently posted as PR stack:
graphql/graphql-js#3894 => introduces Publisher
graphql/graphql-js#3886 => bulk of effort, introduces deduplication
graphql/graphql-js#3897 => adds pending

These spec edits do not currently include optional follow-on:
graphql/graphql-js#3895 => payload consolidation

[The diff to main might be helpful, but this is built on top of the amazing #742 and so the diff from that branch could be more useful.]

IMPORTANT NOTE:

These are hopefully trending toward complete in terms of the algorithm -- more explanatory prose should definitely be added.


TLDR?

The implementation and spec changes show how one can start executing deferred fragments semi-immediately (i.e. after deferring in an implementation-specific way), rather than waiting for the entire initial result to be emitted. This is not required -- one could still be compliant with the spec by deferring all the way until the initial result completes! In fact, how one defers is not per se observable and so the spec cannot mandate much about it with great normative force. But -- and I think this is important -- this PR and the implementation PR provide an algorithm/implementation/spec changes that give servers the flexibility to do what they think is right in that regard, and that might be desirable.

As of this moment, I am fairly confident in the implementation PR over at graphql-js, and the spec PR should generally correspond, demonstrating:

= the Field Group and Defer Usage record types that contain the information derived from the operation during field collection

= the introduction of a distinction between Incremental Data records and Subsequent Result records. Deferred Fragment records and Stream Items records exemplify Subsequent Results that are sent as a group to the client. But an individual Deferred Fragment Record may consist of a number of distinct Deferred Grouped Field Set records, which may overlap with other Deferred Fragment Records and should not be sent more than once. Deferred Grouped Field Set records are therefore a unit of Incremental Data and are tracked with a new record type. Stream Items records always contain a single unit of incremental data that is sent only once with little complication; they therefore represent both Subsequent Result and Incremental Data Records.

= the new Publisher construct, with a set of algorithms that create and manipulate Subsequent Result and Incremental Data records. Mutation of these records is not performed directly during execution, but only via interaction with the Publisher.

= the deferMap, which maps Defer Usage records to individual Deferred Fragment Subsequent Result records

** huge thanks to @Urigo and @dotansimha of the guild for sponsoring my open-source work on this. **

robrichard and others added 30 commits January 15, 2023 12:15
Update Section 3 -- Type System.md

Update Section 3 -- Type System.md

Update Section 3 -- Type System.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Update Section 6 -- Execution.md

Amend changes

change initial_count to initialCount

add payload fields to Response section

add stream validation for overlapping fields

spelling updates

add note about re-execution

add note about final payloads

label is optional

fix build

Update ExecuteQuery with hasNext logic

fix spelling

fix spaces

Update execution to add defer/stream to mutations and subscriptions

clarify stream records

Apply suggestions from code review

Co-authored-by: Benjie Gillam <benjie@jemjie.com>

missing bracket

Update spec/Section 7 -- Response.md

Co-authored-by: Benjie Gillam <benjie@jemjie.com>

clarify line about stream record iterator

update visitedFragments with defer

Updates to consolidate subsequent payload logic for queries, mutations, and subscriptions

Apply suggestions from code review

Co-authored-by: Benjie Gillam <benjie@jemjie.com>

address review feedback

Add handling of termination signal

more formatting

fix spelling

Add assertion for record type

add "Stream Directives Are Used On List Fields" validation rule

Add defaultValue to @stream initialCount

Update spec/Section 5 -- Validation.md

Co-authored-by: Benjie Gillam <benjie@jemjie.com>

# Conflicts:
#	spec/Section 3 -- Type System.md
#	spec/Section 5 -- Validation.md
#	spec/Section 6 -- Execution.md
#	spec/Section 7 -- Response.md
# Conflicts:
#	spec/Section 6 -- Execution.md
# Conflicts:
#	spec/Section 3 -- Type System.md
# Conflicts:
#	spec/Section 3 -- Type System.md
# Conflicts:
#	spec/Section 3 -- Type System.md
# Conflicts:
#	spec/Section 3 -- Type System.md
# Conflicts:
#	spec/Section 7 -- Response.md
# Conflicts:
#	spec/Section 7 -- Response.md
# Conflicts:
#	spec/Section 3 -- Type System.md
#	spec/Section 6 -- Execution.md
# Conflicts:
#	spec/Section 7 -- Response.md
# Conflicts:
#	spec/Section 6 -- Execution.md
# Conflicts:
#	spec/Section 6 -- Execution.md
# Conflicts:
#	spec/Section 6 -- Execution.md
…ators

# Conflicts:
#	spec/Section 6 -- Execution.md
@yaacovCR
Copy link
Contributor Author

Pushed some fixes — see graphql/graphql-js#3886 (comment)

@yaacovCR
Copy link
Contributor Author

closing in favor of #1034

@yaacovCR yaacovCR closed this Jul 12, 2023
@benjie benjie added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants