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

Disc 5634 pluggable document store #5644

Merged

Conversation

owenallenaz
Copy link
Contributor

This was discussed in #5634 . I wasn't sure if you wanted me to revoke the experimental_approximateDocumentStoreMiB or, since that would become a breaking change, if you wanted to keep it out of this PR. I kept it, so that it functions exactly as it did prior. If we are going to keep it, we should probably add a test to the documentStore test file to verify it's actually passing in correctly.

Couple key things I want to call out:

  • Added a document entry, it's a little verbose, but this feature doesn't seem to warrant it's own page, and it really should be rarely used, so it seems good enough.
  • I added a DocumentStore to types.ts this way we aren't re-specifying the contents of documentStore across many different files. It's definition is using KeyValueCache as suggested.
  • The feature supports true, false, undefined or custom object. The downstream code already handled an undefined store luckily so I didn't have to change that at all.
  • I am normalizing the value of documentStore so that downstream code always receives a DocumentStore rather than the DocumentStore | boolean complexity that the external constructor accepts.
  • To test it I needed to a extension of the main ApolloServerBase so that I could externally call the protected graphQLServerOptions. There might be a better way to accomplish this...

Let me know what you'd like to tweak.

@glasser glasser force-pushed the DISC-5634-pluggable-document-store branch from bdf7419 to 450e422 Compare October 1, 2021 19:42
@glasser glasser force-pushed the DISC-5634-pluggable-document-store branch from 450e422 to 1a49357 Compare October 1, 2021 19:43
@glasser
Copy link
Member

glasser commented Oct 1, 2021

I started by squashing your commits into a single commit and rebasing on main (which involved making a few tweaks based on changes already on main).

Before merging I'm going to do the following:

  • add CHANGELOG entry
  • minor copy-edits to docs
  • Remove experimental_approximateDocumentStoreMiB and document replacement in CHANGELOG (what's the point of marking things as experimental if you can't take them away when there are replacements?)
  • Remove ability to set store as boolean. I think null is more appropriate here.

- Remove `experimental_approximateDocumentStoreMiB` (but add
  InMemoryLRUCache.jsonBytesSizeCalculator which makes it easier to
  reproduce)
- Remove `documentStore: boolean`; instead `documentStore: null`
@glasser
Copy link
Member

glasser commented Oct 1, 2021

OK, feeling good about this. I probably won't do the actual merge and release until Monday; if you object to any of the changes I've made please let me know @owenallenaz!

There's some formatting issues with the code snippet I added in the API reference doc page. @trevorblades @StephenBarlow thoughts?

@glasser glasser self-assigned this Oct 1, 2021
@glasser glasser added 2021-10 and removed 2021-09 labels Oct 1, 2021

To use `InMemoryLRUCache` but change its size to an amount `approximateDocumentStoreMiB`:

<div style="max-width: 400px;">
Copy link
Member

Choose a reason for hiding this comment

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

It scrolls sideways but at least only this scrolls sideways.

Copy link
Contributor

Choose a reason for hiding this comment

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

haha yes, in a world where this codeblock is helpful, this is the preferable solution for its rendering

@glasser glasser merged commit b2c37ae into apollographql:main Oct 5, 2021
@glasser
Copy link
Member

glasser commented Mar 24, 2022

Unfortunately, if this feature was used in Apollo Gateway (ie, any new ApolloServer call receiving both non-null/undefined documentStore and gateway options), it could lead to a bug where invalid operations could be executed after a schema update. If you are using this feature in a Gateway, we highly recommend upgrading to v3.6.6 which fixes the issue.

The fix is that we now always prefix keys in the document store with a random string that is refreshed each time a new schema is received. We apply this fix in the non-Gateway case too. The fix to the issue also means that the caveat in the docs about not sharing a documentSource across ApolloServer objects has been removed, as this random prefix helps with that case as well.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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.

None yet

3 participants