-
Notifications
You must be signed in to change notification settings - Fork 926
Wire together LRU garbage collection #1392
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you'll need my approval for this PR anymore, but just in case, this LGTM for @firebase/testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the super slow review. This basically LGTM but I left a few nits and one potential bug.
return this._firestoreClient.start(persistenceSettings); | ||
return this._firestoreClient.start( | ||
persistenceSettings, | ||
this._config.settings.cacheSizeBytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tempting to have an InternalPersistenceSettings type that includes cacheSizeBytes, since it looks pretty strange to have them separate, and we're always passing them together... WDYT?
The other option would be to rename persistenceSettings stuff to indexedDbPersistenceSettings or something, but I'm not really excited about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like InternalPersistenceSettings
might work. I'll do that in a separate PR against this one to see what it looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR here: #1411
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple remaining small nits. I'll look at #1411 next.
packages/firestore/test/unit/local/lru_garbage_collector.test.ts
Outdated
Show resolved
Hide resolved
* Refactor PersistenceSettings
Port of firebase/firebase-android-sdk#68
Enables LRU garbage collection, sets the default theshold at 40mb.