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

Fixes experimental_approximateDocumentStoreMiB #5629

Conversation

owenallenaz
Copy link
Contributor

The ApolloServer documentation calls out a setting for experimental_approximateDocumentStoreMiB allowing you to set max size of the internal documentStore.

Current code for initializeDocumentStore:

private initializeDocumentStore(): InMemoryLRUCache<DocumentNode> {
    return new InMemoryLRUCache<DocumentNode>({
      // Create ~about~ a 30MiB InMemoryLRUCache.  This is less than precise
      // since the technique to calculate the size of a DocumentNode is
      // only using JSON.stringify on the DocumentNode (and thus doesn't account
      // for unicode characters, etc.), but it should do a reasonable job at
      // providing a caching document store for most operations.
      maxSize:
        Math.pow(2, 20) * (this.experimental_approximateDocumentStoreMiB || 30),
      sizeCalculator: approximateObjectSize,
    });
  }

That code relies on this.experimental_approximateDocumentStoreMiB , but that variable is never set in the constructor. This PR fixes that.

That being said, I would actually like to go one step further, to allow passing a custom documentStore as part of the constructor. One of our graphql server processes maintains 20 different ApolloServer that host different versions of schema-stitched APIs (cms-v1, cms-v2, cms-v3, dms-v1 etc). It would be nice if I could re-use the document store between all of those instances since there is considerable overlap in the queries between the different versions of the graphql servers. Right now it gobbles up memory because we end up with 30mb x 20 servers. I didn't want to do that step, because that's really more of a feature request, but I'd be willing to update this PR, or spin a new one to include that functionality if it's acceptable.

@apollo-cla
Copy link

@owenallenaz: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@@ -177,6 +177,8 @@ export class ApolloServerBase<

this.parseOptions = parseOptions;
this.context = context;
this.experimental_approximateDocumentStoreMiB =
Copy link
Member

Choose a reason for hiding this comment

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

Ah, good catch! I would have thought that TS would give a noUnusedLocals error on the declaration of experimental_approximateDocumentStoreMiB but its existence affects the contents of requestOptions.

@glasser
Copy link
Member

glasser commented Aug 19, 2021

I'll merge this improvement for now. I think it would be reasonable to offer a new documentStorageCache:KeyValueCache constructor argument, which I think could just replace experimental_approximateDocumentStoreMiB entirely. Are you able to implement that? I think it would be perfectly reasonable to entirely remove an option that is labeled as experimental and has never worked.

@glasser glasser merged commit f22be38 into apollographql:main Aug 19, 2021
@owenallenaz owenallenaz deleted the allows-experimental_approximateDocumentStoreMiB-to-work branch August 23, 2021 16:45
@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.

3 participants