Enable LRU GC #68

gsoltis · 2018-10-09T22:51:24Z

Don't merge until LGTMs are collected on API proposal.

Implements threshold and scheduling for LRU GC and sets up default behavior.
Fixes bug with memory LRU persistence where all documents were being iterated rather than just orphaned documents.

Port of firebase/firebase-ios-sdk#1905, although now this one is a little ahead since it sets GC to run by default.

gsoltis · 2018-10-09T23:29:21Z

/test connected-check

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestoreSettings.java

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java

...e-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java

firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java

schmidt-sebastian

Some (somewhat optional) comments remaining. Thanks for the changes!

schmidt-sebastian · 2018-10-15T18:44:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestoreSettings.java

@@ -24,6 +24,14 @@
 /** Settings used to configure a FirebaseFirestore instance. */
 @PublicApi
 public final class FirebaseFirestoreSettings {
+  /**
+   * Set this constant as the value for {@link


Nit: Maybe Constant to use with , since we want to describe what the value is, rather than what someone can do with it.

schmidt-sebastian · 2018-10-15T18:49:30Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

+              params);
+      lruDelegate = sqlitePersistence.getReferenceDelegate();
+      persistence = sqlitePersistence;
+      scheduleLruGarbageCollection();


We can look at this more in the future, but it might actually make sense to split these up, using the same argument that you provided: In FirebaseFirestoreSettings/FirestoreClient we can validate whether the settings for the GC algorithm make sense. In the GC code, we can then always assume that all values make sense and we don't have to special case anything.

That's just my two cents and your mileage may vary. I believe that for now the PR could be simplified if we didn't have to differentiate between -1 and >0 in the LRU GC.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java

schmidt-sebastian

LGTM, with an optional "idea" on how to remove the scheduling if GC is disabled, while still keeping everything encapsulated.

schmidt-sebastian · 2018-10-20T01:32:58Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

+              params);
+      lruDelegate = sqlitePersistence.getReferenceDelegate();
+      persistence = sqlitePersistence;
+      scheduleLruGarbageCollection();


Hm, I think that would indeed be a compromise, and it's one that I am less happy with than what we currently have. I prefer the current solution over asking GC whether it is enabled and then acting upon this by asking GC to do its job.

I thought of another approach though - can we passed the AsyncQueue to the GC algorithm and let it schedule itself if it needs to?

* Use orphaned documents in sequence number calculation

* Encapsulate scheduling in an inner class

gsoltis · 2018-11-08T18:01:02Z

/test smoke-tests-release

gsoltis · 2018-11-20T23:20:32Z

/test smoke-tests-debug

gsoltis · 2018-11-27T22:42:57Z

/retest

gsoltis · 2018-12-04T21:49:07Z

/retest

gsoltis · 2018-12-18T23:51:42Z

/retest

* Switch default for LRU collection to disabled

google-oss-bot · 2018-12-19T22:41:17Z

@gsoltis: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests-release	`793b0b4`	link	`/test smoke-tests-release`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Greg Soltis added 2 commits October 9, 2018 14:27

Enable LRU GC

4aa7357

Add minimum cache size

6163a63

gsoltis assigned schmidt-sebastian Oct 9, 2018

gsoltis requested a review from schmidt-sebastian October 9, 2018 22:51

google-oss-bot added the size/XL label Oct 9, 2018

Formatting

b392c4f

schmidt-sebastian suggested changes Oct 11, 2018

View reviewed changes

schmidt-sebastian assigned gsoltis and unassigned schmidt-sebastian Oct 11, 2018

Greg Soltis added 4 commits October 12, 2018 08:22

Add public api and comment to CACHE_SIZE_UNLIMITED

096a9d2

Review feedback

2528b78

Update comment on cache size

b406fd4

Formatting

cdbfa86

gsoltis assigned schmidt-sebastian and unassigned gsoltis Oct 12, 2018

Update comment

a67b447

schmidt-sebastian reviewed Oct 15, 2018

View reviewed changes

schmidt-sebastian assigned gsoltis and unassigned schmidt-sebastian Oct 15, 2018

Greg Soltis added 4 commits October 17, 2018 10:22

Update comment

358ded8

Merge branch 'master' into enable_lru

0b5fb37

Update comment

8cb050a

Formatting

9244537

schmidt-sebastian approved these changes Oct 20, 2018

View reviewed changes

Greg Soltis added 2 commits October 23, 2018 10:20

Use orphaned docs as part of GC calculation (#80)

b87a3cd

* Use orphaned documents in sequence number calculation

Encapsulate scheduling in an inner class (#111)

ca8e8a9

* Encapsulate scheduling in an inner class

googlebot added the cla: yes Override cla label Nov 8, 2018

saurabhdtu mentioned this pull request Nov 20, 2018

[Firestore] SQLite error: duplicate column name: target_count #134

Closed

Greg Soltis added 2 commits November 20, 2018 14:54

Merge branch 'master' into enable_lru

38a35ac

Remove errant gradle stuff

1a53dfd

gsoltis mentioned this pull request Nov 20, 2018

Wire together LRU garbage collection firebase/firebase-js-sdk#1392

Merged

Merge in master

83f7df5

Merge branch 'master' into enable_lru

1e97139

gsoltis mentioned this pull request Dec 4, 2018

connected-check-changed CI test is flaky #152

Closed

Merge branch 'master' into enable_lru

0979892

Switch default for LRU collection to disabled (#174)

793b0b4

* Switch default for LRU collection to disabled

gsoltis merged commit c627b48 into master Dec 19, 2018

gsoltis deleted the gsoltis/enable_lru branch December 19, 2018 23:11

firebase locked and limited conversation to collaborators Oct 14, 2019

Enable LRU GC #68

Enable LRU GC #68

Uh oh!

Conversation

gsoltis commented Oct 9, 2018

Uh oh!

gsoltis commented Oct 9, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

gsoltis Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoltis commented Nov 8, 2018

Uh oh!

gsoltis commented Nov 20, 2018

Uh oh!

gsoltis commented Nov 27, 2018

Uh oh!

gsoltis commented Dec 4, 2018

Uh oh!

gsoltis commented Dec 18, 2018

Uh oh!

google-oss-bot commented Dec 19, 2018

Uh oh!

Uh oh!

schmidt-sebastian Oct 20, 2018 •

edited

Loading