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

Wire together LRU garbage collection #1392

Merged
merged 21 commits into from
Dec 5, 2018
Merged

Wire together LRU garbage collection #1392

merged 21 commits into from
Dec 5, 2018

Conversation

gsoltis
Copy link

@gsoltis gsoltis commented Nov 20, 2018

Port of firebase/firebase-android-sdk#68

Enables LRU garbage collection, sets the default theshold at 40mb.

Copy link
Contributor

@ryanpbrewster ryanpbrewster left a 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

@gsoltis gsoltis assigned mikelehen and unassigned gsoltis Nov 28, 2018
Copy link
Contributor

@mikelehen mikelehen left a 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.

packages/firestore/src/api/database.ts Show resolved Hide resolved
packages/firestore/src/api/database.ts Outdated Show resolved Hide resolved
return this._firestoreClient.start(persistenceSettings);
return this._firestoreClient.start(
persistenceSettings,
this._config.settings.cacheSizeBytes
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

PR here: #1411

packages/firestore/src/local/local_store.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/lru_garbage_collector.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/lru_garbage_collector.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/lru_garbage_collector.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/lru_garbage_collector.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/memory_persistence.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/memory_persistence.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned gsoltis and unassigned mikelehen Nov 30, 2018
@gsoltis gsoltis assigned mikelehen and unassigned gsoltis Dec 4, 2018
Copy link
Contributor

@mikelehen mikelehen left a 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/src/api/database.ts Outdated Show resolved Hide resolved
packages/firestore/src/local/lru_garbage_collector.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned gsoltis and unassigned mikelehen Dec 4, 2018
@gsoltis gsoltis merged commit 28bb3ad into master Dec 5, 2018
@gsoltis gsoltis deleted the gsoltis/enable_lru branch December 5, 2018 00:10
@firebase firebase locked and limited conversation to collaborators Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants