Skip to content

Don't write to persisted query cache until execution will begin.#2227

Merged
abernix merged 2 commits intomasterfrom
abernix/relocate-persisted-query-write
Jan 25, 2019
Merged

Don't write to persisted query cache until execution will begin.#2227
abernix merged 2 commits intomasterfrom
abernix/relocate-persisted-query-write

Conversation

@abernix
Copy link
Copy Markdown
Member

@abernix abernix commented Jan 25, 2019

Since we already write to the persisted query cache asynchronously (and
intentionally do not await the Promise) this won't have any affect on the
current behavior of when the persisted query cache is written to in the
event of an executable request, but if an operation comes in and doesn't
parse or validate, we'll avoid wasting cache space on an operation that will
never execute.

Additionally, in a similar light, if a plugin throws an error which stops
execution, we can avoid the side-effect of writing to the persisted query
cache.

In terms of the APQ behavior, while this could cause additional round-trips
for a client which repeatedly sends an invalid operation, that operation is
never going to successfully finish anyway. While developer tooling will
help avoid this problem in the first place, this refusal to store an invalid
operation in the APQ cache could actually help diagnose a failure since the
full operation (rather than just the SHA256 of that document) will appear
in the browser's dev-tools on the retry.

If we're looking to spare parsing and validating documents which we know are
going to fail, I think that's going to be a better use of the (new)
documentStore cache (#2111), since its in-memory and can accommodate a
more complex data structure which includes the validation errors from the
previous attempt which will, of course, be the same when validating the same
operation against the same schema again. As the PR that introduced that
feature shows, sparing those additional parses and validations (cached APQ
documents still needs to be parsed and validated, currently) will provide a
more successful performance win overall.

Ref: #2111

Since we already write to the persisted query cache asynchronously (and
intentionally do not await the Promise) this won't have any affect on the
current behavior of when the persisted query cache is written to in the
event of an executable request, but if an operation comes in and doesn't
parse or validate, we'll avoid wasting cache space on an operation that will
never execute.

Additionally, in a similar light, if a plugin throws an error which stops
execution, we can avoid the side-effect of writing to the persisted query
cache.

In terms of the APQ behavior, while this could cause additional round-trips
for a client which repeatedly sends an invalid operation, that operation is
never going to successfully finish anyway.  While developer tooling will
help avoid this problem in the first place, this refusal to store an invalid
operation in the APQ cache could actually help diagnose a failure since the
full operation (rather than just the SHA256 of that document) will appear
in the browser's dev-tools on the retry.

If we're looking to spare parsing and validating documents which we know are
going to fail, I think that's going to be a better use of the (new)
`documentStore` cache (#2111), since its in-memory and can accommodate a
more complex data structure which includes the validation errors from the
previous attempt which will, of course, be the same when validating the same
operation against the same schema again.  As the PR that introduced that
feature shows, sparing those additional parses and validations (cached APQ
documents still needs to be parsed and validated, currently) will provide a
more successful performance win overall.

Ref: #2111
@abernix abernix force-pushed the abernix/relocate-persisted-query-write branch from 6c87e03 to a643f00 Compare January 25, 2019 15:38
@abernix abernix added this to the Release 2.3.2 milestone Jan 25, 2019
@abernix abernix merged commit 18d9041 into master Jan 25, 2019
@abernix abernix deleted the abernix/relocate-persisted-query-write branch January 28, 2019 16:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant