Skip to content

Commit 6c6698e

Browse files
cherylEnkiducherylEnkidu
committed
address more feedbacks
1 parent 48d649f commit 6c6698e

File tree

10 files changed

+82
-101
lines changed

10 files changed

+82
-101
lines changed

‎firebase-firestore/src/androidTest/java/com/google/firebase/firestore/IndexingTest.java

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirestore;
1919
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor;
2020
import static com.google.firebase.firestore.testutil.TestUtil.assertDoesNotThrow;
21+
import static com.google.firebase.firestore.testutil.TestUtil.expectError;
2122
import static com.google.firebase.firestore.testutil.TestUtil.map;
2223
import static org.junit.Assert.assertEquals;
2324

@@ -108,7 +109,7 @@ public void testBadIndexDoesNotCrashClient() {
108109
}
109110

110111
@Test
111-
public void testAutomaticIndexingSetSuccessfully() {
112+
public void testAutoIndexCreationSetSuccessfully() {
112113
// Use persistent disk cache (default)
113114
FirebaseFirestore db = testFirestore();
114115
FirebaseFirestoreSettings settings =
@@ -138,7 +139,7 @@ public void testAutomaticIndexingSetSuccessfully() {
138139
}
139140

140141
@Test
141-
public void testAutomaticIndexingSetSuccessfullyUseDefault() {
142+
public void testAutoIndexCreationSetSuccessfullyUsingDefault() {
142143
// Use persistent disk cache (default)
143144
FirebaseFirestore db = testFirestore();
144145

@@ -161,4 +162,18 @@ public void testAutomaticIndexingSetSuccessfullyUseDefault() {
161162
results = waitFor(collection.whereEqualTo("match", true).get());
162163
assertEquals(1, results.size());
163164
}
165+
166+
@Test
167+
public void testAutoIndexCreationAfterFailsTermination() {
168+
FirebaseFirestore db = testFirestore();
169+
waitFor(db.terminate());
170+
171+
expectError(
172+
() -> db.getPersistentCacheIndexManager().enableIndexAutoCreation(),
173+
"The client has already been terminated");
174+
175+
expectError(
176+
() -> db.getPersistentCacheIndexManager().disableIndexAutoCreation(),
177+
"The client has already been terminated");
178+
}
164179
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@
4949
import com.google.firebase.firestore.model.ResourcePath;
5050
import com.google.firebase.firestore.remote.FirestoreChannel;
5151
import com.google.firebase.firestore.remote.GrpcMetadataProvider;
52-
import com.google.firebase.firestore.util.Assert;
5352
import com.google.firebase.firestore.util.AsyncQueue;
5453
import com.google.firebase.firestore.util.ByteBufferInputStream;
5554
import com.google.firebase.firestore.util.Executors;
@@ -107,8 +106,7 @@ public interface InstanceRegistry {
107106
private volatile FirestoreClient client;
108107
private final GrpcMetadataProvider metadataProvider;
109108

110-
@Nullable
111-
private AtomicReference<PersistentCacheIndexManager> persistentCacheIndexManager =
109+
private final AtomicReference<PersistentCacheIndexManager> persistentCacheIndexManager =
112110
new AtomicReference<>();
113111

114112
@NonNull
@@ -422,12 +420,11 @@ public Task<Void> setIndexConfiguration(@NonNull String json) {
422420
@Nullable
423421
public PersistentCacheIndexManager getPersistentCacheIndexManager() {
424422
ensureClientConfigured();
425-
Assert.hardAssertNonNull(
426-
persistentCacheIndexManager, "persistentCacheIndexManager has not been initialized.");
427423

428-
if (settings.isPersistenceEnabled()
429-
|| settings.getCacheSettings() instanceof PersistentCacheSettings) {
430-
persistentCacheIndexManager.compareAndSet(null, new PersistentCacheIndexManager(client));
424+
if ((settings.isPersistenceEnabled()
425+
|| settings.getCacheSettings() instanceof PersistentCacheSettings)
426+
&& persistentCacheIndexManager.get() == null) {
427+
persistentCacheIndexManager.set(new PersistentCacheIndexManager(client));
431428
}
432429

433430
return persistentCacheIndexManager.get();

‎firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,14 @@ public final class PersistentCacheIndexManager {
4141
* <p>This feature is disabled by default.
4242
*/
4343
public void enableIndexAutoCreation() {
44-
client.enableIndexAutoCreation();
44+
client.setIndexAutoCreationEnabled(true);
4545
}
4646

4747
/**
4848
* Stops creating persistent cache indexes automatically for local query execution. The indexes
4949
* which have been created by calling {@link #enableIndexAutoCreation()} still take effect.
5050
*/
5151
public void disableIndexAutoCreation() {
52-
client.disableIndexAutoCreation();
52+
client.setIndexAutoCreationEnabled(false);
5353
}
5454
}

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -353,14 +353,9 @@ public Task<Void> configureFieldIndexes(List<FieldIndex> fieldIndices) {
353353
return asyncQueue.enqueue(() -> localStore.configureFieldIndexes(fieldIndices));
354354
}
355355

356-
public void enableIndexAutoCreation() {
356+
public void setIndexAutoCreationEnabled(boolean enabled) {
357357
verifyNotTerminated();
358-
asyncQueue.enqueueAndForget(() -> localStore.enableIndexAutoCreation());
359-
}
360-
361-
public void disableIndexAutoCreation() {
362-
verifyNotTerminated();
363-
asyncQueue.enqueueAndForget(() -> localStore.disableIndexAutoCreation());
358+
asyncQueue.enqueueAndForget(() -> localStore.setIndexAutoCreationEnabled(enabled));
364359
}
365360

366361
public void removeSnapshotsInSyncListener(EventListener<Void> listener) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -802,12 +802,8 @@ public void configureFieldIndexes(List<FieldIndex> newFieldIndexes) {
802802
});
803803
}
804804

805-
public void enableIndexAutoCreation() {
806-
queryEngine.enableIndexAutoCreation();
807-
}
808-
809-
public void disableIndexAutoCreation() {
810-
queryEngine.disableIndexAutoCreation();
805+
public void setIndexAutoCreationEnabled(boolean enabled) {
806+
queryEngine.setIndexAutoCreationEnabled(enabled);
811807
}
812808

813809
/** Mutable state for the transaction in allocateQuery. */

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java

Lines changed: 26 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -62,39 +62,34 @@
6262
public class QueryEngine {
6363
private static final String LOG_TAG = "QueryEngine";
6464

65-
private static final int MIN_COLLECTION_SIZE_TO_AUTO_CREATE_INDEX = 100;
65+
private static final int INDEX_AUTO_CREATION_DEFAULT_MIN_COLLECTION_SIZE = 100;
6666

67-
// TODO(csi): update the temp cost.
6867
/**
6968
* This cost represents the evaluation result of (([index, docKey] + [docKey, docContent]) per
7069
* document in the result set) / ([docKey, docContent] per documents in full collection scan)
7170
* coming from experiment https://github.com/firebase/firebase-android-sdk/pull/5064.
7271
*/
73-
private static final int RELATIVE_INDEX_READ_COST = 3;
72+
private static final double DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT = 3;
7473

7574
private LocalDocumentsView localDocumentsView;
7675
private IndexManager indexManager;
7776
private boolean initialized;
7877

79-
private boolean automaticIndexingEnabled = false;
78+
private boolean indexAutoCreationEnabled = false;
8079

8180
/** SDK only decides whether it should create index when collection size is larger than this. */
82-
private int minCollectionSizeToAutoCreateIndex = MIN_COLLECTION_SIZE_TO_AUTO_CREATE_INDEX;
81+
private int indexAutoCreationMinCollectionSize = INDEX_AUTO_CREATION_DEFAULT_MIN_COLLECTION_SIZE;
8382

84-
private int relativeIndexReadCost = RELATIVE_INDEX_READ_COST;
83+
private double relativeIndexReadCostPerDocument = DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT;
8584

8685
public void initialize(LocalDocumentsView localDocumentsView, IndexManager indexManager) {
8786
this.localDocumentsView = localDocumentsView;
8887
this.indexManager = indexManager;
8988
this.initialized = true;
9089
}
9190

92-
public void enableIndexAutoCreation() {
93-
this.automaticIndexingEnabled = true;
94-
}
95-
96-
public void disableIndexAutoCreation() {
97-
this.automaticIndexingEnabled = false;
91+
public void setIndexAutoCreationEnabled(boolean enabled) {
92+
this.indexAutoCreationEnabled = enabled;
9893
}
9994

10095
public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
@@ -115,7 +110,7 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
115110

116111
QueryContext context = new QueryContext();
117112
result = executeFullCollectionScan(query, context);
118-
if (result != null && automaticIndexingEnabled) {
113+
if (result != null && indexAutoCreationEnabled) {
119114
createCacheIndexes(query, context, result.size());
120115
}
121116
return result;
@@ -127,37 +122,32 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
127122
*/
128123
// TODO(csi): Auto experiment data.
129124
private void createCacheIndexes(Query query, QueryContext context, int resultSize) {
130-
if (context.getDocumentReadCount() < minCollectionSizeToAutoCreateIndex) {
125+
if (context.getDocumentReadCount() < indexAutoCreationMinCollectionSize) {
131126
Logger.debug(
132127
LOG_TAG,
133128
"SDK will only creates cache indexes for collection contains more than or equal to "
134129
+ "%s documents.",
135-
minCollectionSizeToAutoCreateIndex);
130+
indexAutoCreationMinCollectionSize);
136131
return;
137132
}
138133

139-
if (Logger.isDebugEnabled()) {
140-
Logger.debug(
141-
LOG_TAG,
142-
"Query scans %s local documents and returns %s documents as results.",
143-
context.getDocumentReadCount(),
144-
resultSize);
145-
}
134+
Logger.debug(
135+
LOG_TAG,
136+
"Query scans %s local documents and returns %s documents as results.",
137+
context.getDocumentReadCount(),
138+
resultSize);
146139

147-
String decisionStr = "";
148-
if (context.getDocumentReadCount() > relativeIndexReadCost * resultSize) {
140+
if (context.getDocumentReadCount() > relativeIndexReadCostPerDocument * resultSize) {
149141
indexManager.createTargetIndices(query.toTarget());
142+
Logger.debug(
143+
LOG_TAG,
144+
"The SDK decides to create cache indexes for this query, as using cache indexes "
145+
+ "may help improve performance.");
150146
} else {
151-
decisionStr = " not";
152-
}
153-
154-
if (Logger.isDebugEnabled()) {
155147
Logger.debug(
156148
LOG_TAG,
157-
"The SDK decides%s to create cache indexes for this query, as using cache indexes "
158-
+ "may%s help improve performance.",
159-
decisionStr,
160-
decisionStr);
149+
"The SDK decides not to create cache indexes for this query, as using cache indexes "
150+
+ "may not help improve performance.");
161151
}
162152
}
163153

@@ -336,12 +326,12 @@ private ImmutableSortedMap<DocumentKey, Document> appendRemainingResults(
336326
}
337327

338328
@VisibleForTesting
339-
void setMinCollectionSizeToAutoCreateIndex(int newMin) {
340-
minCollectionSizeToAutoCreateIndex = newMin;
329+
void setIndexAutoCreationMinCollectionSize(int newMin) {
330+
indexAutoCreationMinCollectionSize = newMin;
341331
}
342332

343333
@VisibleForTesting
344-
void setRelativeIndexReadCost(int newCost) {
345-
relativeIndexReadCost = newCost;
334+
void setRelativeIndexReadCostPerDocument(double newCost) {
335+
relativeIndexReadCostPerDocument = newCost;
346336
}
347337
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,6 @@ private Map<DocumentKey, MutableDocument> getAll(
223223
row -> {
224224
processRowInBackground(backgroundQueue, results, row, filter);
225225
if (context != null) {
226-
// Increases the counter by 1 for every document processed.
227226
context.incrementDocumentReadCount();
228227
}
229228
});

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,23 +88,18 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
8888
}
8989

9090
@Override
91-
public void enableIndexAutoCreation() {
92-
queryEngine.enableIndexAutoCreation();
91+
public void setIndexAutoCreationEnabled(boolean enabled) {
92+
queryEngine.setIndexAutoCreationEnabled(enabled);
9393
}
9494

9595
@Override
96-
public void disableIndexAutoCreation() {
97-
queryEngine.disableIndexAutoCreation();
96+
public void setIndexAutoCreationMinCollectionSize(int newMin) {
97+
queryEngine.setIndexAutoCreationMinCollectionSize(newMin);
9898
}
9999

100100
@Override
101-
public void setMinCollectionSizeToAutoCreateIndex(int newMin) {
102-
queryEngine.setMinCollectionSizeToAutoCreateIndex(newMin);
103-
}
104-
105-
@Override
106-
public void setRelativeIndexReadCost(int newCost) {
107-
queryEngine.setRelativeIndexReadCost(newCost);
101+
public void setRelativeIndexReadCostPerDocument(double newCost) {
102+
queryEngine.setRelativeIndexReadCostPerDocument(newCost);
108103
}
109104

110105
/**

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -217,27 +217,21 @@ protected void executeQuery(Query query) {
217217
lastQueryResult = localStore.executeQuery(query, /* usePreviousResults= */ true);
218218
}
219219

220-
protected void enableIndexAutoCreation() {
220+
protected void setIndexAutoCreationEnabled(boolean enabled) {
221221
// Noted: there are two queryEngines here, the first one is extended by CountingQueryEngine,
222222
// which is set by localStore function; The second one a pointer inside CountingQueryEngine,
223223
// which is set by queryEngine function.
224224
// Only the second function takes effect in the tests. Adding first one here for compatibility.
225-
localStore.enableIndexAutoCreation();
226-
queryEngine.enableIndexAutoCreation();
227-
}
228-
229-
protected void disableIndexAutoCreation() {
230-
// Please refer to the notes in `enableIndexAutoCreation()`
231-
localStore.disableIndexAutoCreation();
232-
queryEngine.disableIndexAutoCreation();
225+
localStore.setIndexAutoCreationEnabled(enabled);
226+
queryEngine.setIndexAutoCreationEnabled(enabled);
233227
}
234228

235229
protected void setMinCollectionSizeToAutoCreateIndex(int newMin) {
236-
queryEngine.setMinCollectionSizeToAutoCreateIndex(newMin);
230+
queryEngine.setIndexAutoCreationMinCollectionSize(newMin);
237231
}
238232

239-
protected void setRelativeIndexReadCost(int newCost) {
240-
queryEngine.setRelativeIndexReadCost(newCost);
233+
protected void setRelativeIndexReadCostPerDocument(double newCost) {
234+
queryEngine.setRelativeIndexReadCostPerDocument(newCost);
241235
}
242236

243237
private void releaseTarget(int targetId) {

0 commit comments

Comments
 (0)