Don't write to persisted query cache until execution will begin.#2227
Merged
Don't write to persisted query cache until execution will begin.#2227
Conversation
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
6c87e03 to
a643f00
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)
documentStorecache (#2111), since its in-memory and can accommodate amore 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