-
Notifications
You must be signed in to change notification settings - Fork 943
Keep track of number of queries in the query cache #510
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
Changes from 37 commits
eb44929
07e44e7
3e1053f
5e84782
fd50301
be3b463
de237c5
53e56b5
7b3bedb
7b97c0a
c176eff
9154aad
6b072ff
662365f
dd21376
5e54b3a
cb81df8
7991970
2c6d6e1
f16bb4f
038c159
b70303c
34ab25a
f45be5a
44a2da8
fac7d17
55f5ff6
2d955e9
d09153a
b1be9e9
9091d38
31119ec
640ab7c
8527d67
3ad42d8
9da5ecb
6d6a9d1
80077ef
dbf8a7d
62892cf
875595d
6156d25
fbfa133
ba0a2d0
730fe57
9e9a5d1
5d917f1
c479800
28f7c48
8e452fa
edf8376
6d48526
4a85dfa
4a74e36
cc763dd
eb2b0a3
67fe837
860818e
d9b551f
231ef2e
097fdd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,8 +31,7 @@ import { | |
DbTargetDocumentKey, | ||
DbTargetGlobal, | ||
DbTargetGlobalKey, | ||
DbTargetKey, | ||
DbTimestamp | ||
DbTargetKey | ||
} from './indexeddb_schema'; | ||
import { LocalSerializer } from './local_serializer'; | ||
import { PersistenceTransaction } from './persistence'; | ||
|
@@ -56,7 +55,8 @@ export class IndexedDbQueryCache implements QueryCache { | |
private metadata = new DbTargetGlobal( | ||
/*highestTargetId=*/ 0, | ||
/*lastListenSequenceNumber=*/ 0, | ||
SnapshotVersion.MIN.toTimestamp() | ||
SnapshotVersion.MIN.toTimestamp(), | ||
/*targetCount=*/ 0 | ||
); | ||
|
||
/** The garbage collector to notify about potential garbage keys. */ | ||
|
@@ -97,36 +97,75 @@ export class IndexedDbQueryCache implements QueryCache { | |
); | ||
} | ||
|
||
addQueryData( | ||
private saveQueryData( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We usually put private helpers after the public methods that use them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
transaction: PersistenceTransaction, | ||
queryData: QueryData | ||
): PersistencePromise<void> { | ||
const targetId = queryData.targetId; | ||
const addedQueryPromise = targetsStore(transaction).put( | ||
this.serializer.toDbTarget(queryData) | ||
); | ||
if (targetId > this.metadata.highestTargetId) { | ||
this.metadata.highestTargetId = targetId; | ||
return addedQueryPromise.next(() => | ||
globalTargetStore(transaction).put(DbTargetGlobal.key, this.metadata) | ||
); | ||
} else { | ||
return addedQueryPromise; | ||
return targetsStore(transaction).put(this.serializer.toDbTarget(queryData)); | ||
} | ||
|
||
// Updates the in-memory version of the metadata. Saving is done separately. | ||
// Returns true if there were any changes to the metadata. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We usually use /** ... */ for method comments like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
private updateMetadata(queryData: QueryData): boolean { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this helper pays for itself since although it's used twice, we only use the return value once, so the other usage could just be "this.metadata.highestTargetId = Math.Max(...)" If we keep it, can you rename to updateHighestTargetId() to better capture what it's doing and when it'll return true? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the comment, but left the helper. It's being ported from here, which includes the sequence number bit: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Local/FSTLevelDBQueryCache.mm#L139 |
||
let needsUpdate = false; | ||
if (queryData.targetId > this.metadata.highestTargetId) { | ||
this.metadata.highestTargetId = queryData.targetId; | ||
needsUpdate = true; | ||
} | ||
|
||
// TODO(gsoltis): add sequence number check | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh. I guess maybe this is what motivates having this helper? I guess I'd still advocate renaming (eventually to updateTargetIdAndSequenceNumberGlobals or whatever) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was originally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I think this is much clearer! |
||
return needsUpdate; | ||
} | ||
|
||
removeQueryData( | ||
private saveMetadata( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was gonna claim ownership of the term 'metadata' (but Mike already shut me down). Can you make this more query-specific (updateQueryMetadata) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this down with the other private helper methods? (probably next to saveQueryData) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
transaction: PersistenceTransaction | ||
): PersistencePromise<void> { | ||
return globalTargetStore(transaction).put( | ||
DbTargetGlobal.key, | ||
this.metadata | ||
); | ||
} | ||
|
||
addQueryData( | ||
transaction: PersistenceTransaction, | ||
queryData: QueryData | ||
): PersistencePromise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it were me I'd add logic to verify that the query data doesn't already exist (asserting otherwise) as I suggested for the in-memory implementation. This is a perf hit, so I'd add a TODO to revisit for performance. That said, having just the memory implementation enforce the contract is probably sufficient, so I'm okay with leaving it out here. I'd perhaps add a comment to make the expectation clear though, e.g.: // Although we require that the target does not already exist, we don't enforce it here for performance (though MemoryQueryCache does). [and similar for the updateQueryData() method] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not add the read, as in practice we actually do the read before calling this anyways. I am intending on encapsulating this behavior though in the rethink of the interaction between |
||
return this.saveQueryData(transaction, queryData).next(() => { | ||
this.metadata.targetCount += 1; | ||
this.updateMetadata(queryData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned previously, it looks strange to call an updateMetadata() method after we've just manually updated metadata. It's not very clear what it actually does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed the method. |
||
return this.saveMetadata(transaction); | ||
}); | ||
} | ||
|
||
updateQueryData( | ||
transaction: PersistenceTransaction, | ||
queryData: QueryData | ||
): PersistencePromise<void> { | ||
return this.removeMatchingKeysForTargetId( | ||
transaction, | ||
queryData.targetId | ||
).next(() => { | ||
targetsStore(transaction).delete(queryData.targetId); | ||
return this.saveQueryData(transaction, queryData).next(() => { | ||
if (this.updateMetadata(queryData)) { | ||
return this.saveMetadata(transaction); | ||
} else { | ||
return PersistencePromise.resolve(); | ||
} | ||
}); | ||
} | ||
|
||
removeQueryData( | ||
transaction: PersistenceTransaction, | ||
queryData: QueryData | ||
): PersistencePromise<void> { | ||
return this.removeMatchingKeysForTargetId(transaction, queryData.targetId) | ||
.next(() => targetsStore(transaction).delete(queryData.targetId)) | ||
.next(() => { | ||
this.metadata.targetCount -= 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment for removeQueryData() says "no-op if no entry exists". We need to either change the comment (and add appropriate assertions / comments about existence requirement), or change the implementation to check for existence first... As a sanity check, we may want to add a targetCount >= 0 assertion here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment changed. |
||
return this.saveMetadata(transaction); | ||
}); | ||
} | ||
|
||
count(): number { | ||
return this.metadata.targetCount; | ||
} | ||
|
||
getQueryData( | ||
transaction: PersistenceTransaction, | ||
query: Query | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,54 +21,55 @@ import { ResourcePath } from '../model/path'; | |
import { assert } from '../util/assert'; | ||
|
||
import { encode, EncodedResourcePath } from './encoded_resource_path'; | ||
import { wrapRequest } from './simple_db'; | ||
import { PersistencePromise } from './persistence_promise'; | ||
import { SnapshotVersion } from '../core/snapshot_version'; | ||
|
||
export const SCHEMA_VERSION = 1; | ||
|
||
/** Performs database creation and (in the future) upgrades between versions. */ | ||
export function createOrUpgradeDb(db: IDBDatabase, oldVersion: number): void { | ||
assert(oldVersion === 0, 'Unexpected upgrade from version ' + oldVersion); | ||
|
||
db.createObjectStore(DbMutationQueue.store, { | ||
keyPath: DbMutationQueue.keyPath | ||
}); | ||
|
||
// TODO(mikelehen): Get rid of "as any" if/when TypeScript fixes their | ||
// types. https://github.com/Microsoft/TypeScript/issues/14322 | ||
db.createObjectStore( | ||
DbMutationBatch.store, | ||
// tslint:disable-next-line:no-any | ||
{ keyPath: DbMutationBatch.keyPath as any } | ||
); | ||
/** | ||
* Schema Version for the Web client (containing the Mutation Queue, the Query | ||
* and the Remote Document Cache) and target count added to the target global. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment makes no sense to me. If you remove the parenthetical it says, "Schema version for the Web client and target count added to the target global." :-) I'm not sure it's worth enumerating what the schema version represents here. I'd change it to just "Schema version for the Web client" or format it as a history or something:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
*/ | ||
export const SCHEMA_VERSION = 2; | ||
|
||
const targetDocumentsStore = db.createObjectStore( | ||
DbTargetDocument.store, | ||
// tslint:disable-next-line:no-any | ||
{ keyPath: DbTargetDocument.keyPath as any } | ||
); | ||
targetDocumentsStore.createIndex( | ||
DbTargetDocument.documentTargetsIndex, | ||
DbTargetDocument.documentTargetsKeyPath, | ||
{ unique: true } | ||
/** | ||
* Performs database creation and schema upgrades. | ||
* | ||
* Note that in production, this method is only ever used to upgrade the schema | ||
* to SCHEMA_VERSION. Different versions are only used for testing and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "versions" => "toVersion values" ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
* local feature development. | ||
*/ | ||
export function createOrUpgradeDb( | ||
db: IDBDatabase, | ||
txn: IDBTransaction, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point of SimpleDb is to wrap IndexedDb in a nice async-friendly abstraction (by using PersistencePromise, and hiding the nasty onsuccess / onerror wrapping). We've been cheating a little bit by exposing IDBDatabase directly to this code (rather than adding the necessary wrapper functions to SimpleDb) since everything we do on IDBDatabase here is synchronous and so it's not bad to just use the raw IndexedDB API. But now that we're doing async stuff, I think we really want to wrap the upgrade transaction in a PersistenceTransaction and pass that into createOrUpgradeDb() rather than the raw IDBTransaction. This will remove the need for you to pull in wrapRequest() and let you just use the normal SimpleDb API like the rest of our code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
fromVersion: number, | ||
toVersion: number | ||
): PersistencePromise<void> { | ||
// This function currently supports migrating to schema version 1 (Mutation | ||
// Queue, Query and Remote Document Cache) and schema version 2 (Query | ||
// counting). | ||
assert( | ||
fromVersion < toVersion && fromVersion >= 0 && toVersion <= 2, | ||
'Unexpected schema upgrade from v${fromVersion} to v{toVersion}.' | ||
); | ||
|
||
const targetStore = db.createObjectStore(DbTarget.store, { | ||
keyPath: DbTarget.keyPath | ||
}); | ||
// NOTE: This is unique only because the TargetId is the suffix. | ||
targetStore.createIndex( | ||
DbTarget.queryTargetsIndexName, | ||
DbTarget.queryTargetsKeyPath, | ||
{ unique: true } | ||
); | ||
if (fromVersion < 1 && toVersion >= 1) { | ||
createOwnerStore(db); | ||
createMutationQueue(db); | ||
createQueryCache(db); | ||
createRemoteDocumentCache(db); | ||
} | ||
|
||
// NOTE: keys for these stores are specified explicitly rather than using a | ||
// keyPath. | ||
db.createObjectStore(DbDocumentMutation.store); | ||
db.createObjectStore(DbRemoteDocument.store); | ||
db.createObjectStore(DbOwner.store); | ||
db.createObjectStore(DbTargetGlobal.store); | ||
let p = PersistencePromise.resolve(); | ||
if (fromVersion < 2 && toVersion >= 2) { | ||
p = p.next(() => ensureTargetGlobal(txn)).next(() => saveTargetCount(txn)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor but I'd rather combine these two methods and have saveTargetCount just gracefully handle if the row is missing. This 1) Avoids unnecessarily doing a get-put-get-put, and 2) Makes it clearer what is actually new in schema version 2 (the target count). If you want, you can keep ensureTargetGlobal() but make it return the TargetGlobal row, and then call it from within saveTargetCount(). Also, I suggest renaming saveTargetCount() to populateTargetCount() to make it clearer that it needs to do work to figure out what the target count needs to be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the rename... I also still don't think there's a need to have two steps here. TargetGlobal was an existing part of the schema in v1. The only thing you added was a new field to it. Another way to deal with this would be to get rid of ensureTargetGlobal() and have populateTargetCount() just do nothing if the TargetGlobal row doesn't exist yet (since it should get populated correctly as 0 when the TargetGlobal row gets created as a normal part of the client operating). And actually, I think I like this better since 1) it avoids us duplicating the default |
||
} | ||
return p; | ||
} | ||
|
||
// TODO(mikelehen): Get rid of "as any" if/when TypeScript fixes their types. | ||
// https://github.com/Microsoft/TypeScript/issues/14322 | ||
type KeyPath = any; // tslint:disable-line:no-any | ||
|
||
/** | ||
* Wrapper class to store timestamps (seconds and nanos) in IndexedDb objects. | ||
*/ | ||
|
@@ -94,6 +95,10 @@ export class DbOwner { | |
constructor(public ownerId: string, public leaseTimestampMs: number) {} | ||
} | ||
|
||
function createOwnerStore(db: IDBDatabase): void { | ||
db.createObjectStore(DbOwner.store); | ||
} | ||
|
||
/** Object keys in the 'mutationQueues' store are userId strings. */ | ||
export type DbMutationQueueKey = string; | ||
|
||
|
@@ -183,6 +188,18 @@ export class DbMutationBatch { | |
*/ | ||
export type DbDocumentMutationKey = [string, EncodedResourcePath, BatchId]; | ||
|
||
function createMutationQueue(db: IDBDatabase): void { | ||
db.createObjectStore(DbMutationQueue.store, { | ||
keyPath: DbMutationQueue.keyPath | ||
}); | ||
|
||
db.createObjectStore(DbMutationBatch.store, { | ||
keyPath: DbMutationBatch.keyPath as KeyPath | ||
}); | ||
|
||
db.createObjectStore(DbDocumentMutation.store); | ||
} | ||
|
||
/** | ||
* An object to be stored in the 'documentMutations' store in IndexedDb. | ||
* | ||
|
@@ -241,6 +258,10 @@ export class DbDocumentMutation { | |
*/ | ||
export type DbRemoteDocumentKey = string[]; | ||
|
||
function createRemoteDocumentCache(db: IDBDatabase): void { | ||
db.createObjectStore(DbRemoteDocument.store); | ||
} | ||
|
||
/** | ||
* Represents the known absence of a document at a particular version. | ||
* Stored in IndexedDb as part of a DbRemoteDocument object. | ||
|
@@ -451,15 +472,91 @@ export class DbTargetGlobal { | |
* until the backend has caught up to this snapshot version again. This | ||
* prevents our cache from ever going backwards in time. | ||
*/ | ||
public lastRemoteSnapshotVersion: DbTimestamp | ||
public lastRemoteSnapshotVersion: DbTimestamp, | ||
/** | ||
* The number of targets persisted. | ||
*/ | ||
public targetCount: number | ||
) {} | ||
} | ||
|
||
function createQueryCache(db: IDBDatabase): void { | ||
const targetDocumentsStore = db.createObjectStore(DbTargetDocument.store, { | ||
keyPath: DbTargetDocument.keyPath as KeyPath | ||
}); | ||
targetDocumentsStore.createIndex( | ||
DbTargetDocument.documentTargetsIndex, | ||
DbTargetDocument.documentTargetsKeyPath, | ||
{ unique: true } | ||
); | ||
|
||
const targetStore = db.createObjectStore(DbTarget.store, { | ||
keyPath: DbTarget.keyPath | ||
}); | ||
|
||
// NOTE: This is unique only because the TargetId is the suffix. | ||
targetStore.createIndex( | ||
DbTarget.queryTargetsIndexName, | ||
DbTarget.queryTargetsKeyPath, | ||
{ unique: true } | ||
); | ||
db.createObjectStore(DbTargetGlobal.store); | ||
} | ||
|
||
/** | ||
* Counts the number of targets persisted and adds that value to the target | ||
* global singleton. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The codebase usually puts a newline after the description, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
* @param {IDBTransaction} txn The version upgrade transaction for indexeddb | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jsdoc type annotation is unnecessary and no longer correct. :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deleted. |
||
* @return {PersistencePromise<void>} A promise of the operations to add the | ||
* count to the metadata row. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't generally put the param / return types in the doc comments since this is TypeScript... It's redundant / likely to become stale. And I generally omit the "promise" part when documenting return values, so I'd treat this the same as a void function and just drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
*/ | ||
function saveTargetCount(txn: IDBTransaction): PersistencePromise<void> { | ||
const globalStore = txn.objectStore(DbTargetGlobal.store); | ||
return wrapRequest<number>(txn.objectStore(DbTarget.store).count()).next( | ||
(count: number) => | ||
wrapRequest<DbTargetGlobal>(globalStore.get(DbTargetGlobal.key)).next( | ||
(metadata: DbTargetGlobal) => { | ||
metadata.targetCount = count; | ||
return wrapRequest<void>( | ||
globalStore.put(metadata, DbTargetGlobal.key) | ||
); | ||
} | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* The list of all IndexedDB stored used by the SDK. This is used when creating | ||
* transactions so that access across all stores is done atomically. | ||
* Ensures that the target global singleton row exists by adding it if it's | ||
* missing. | ||
* @param {IDBTransaction} txn The version upgrade transaction for indexeddb | ||
* @return {PersistencePromise<void>} A promise of operations to ensure that the | ||
* metadata row exists | ||
*/ | ||
export const ALL_STORES = [ | ||
function ensureTargetGlobal(txn: IDBTransaction): PersistencePromise<void> { | ||
const globalStore = txn.objectStore(DbTargetGlobal.store); | ||
return wrapRequest<DbTargetGlobal | null>( | ||
globalStore.get(DbTargetGlobal.key) | ||
).next((metadata: DbTargetGlobal | null) => { | ||
if (metadata != null) { | ||
return PersistencePromise.resolve(); | ||
} else { | ||
return wrapRequest<void>( | ||
globalStore.put( | ||
new DbTargetGlobal( | ||
/*highestTargetId=*/ 0, | ||
/*lastListenSequenceNumber=*/ 0, | ||
SnapshotVersion.MIN.toTimestamp(), | ||
/*targetCount=*/ 0 | ||
), | ||
DbTargetGlobal.key | ||
) | ||
); | ||
} | ||
}); | ||
} | ||
|
||
// Visible for testing | ||
export const V1_STORES = [ | ||
DbMutationQueue.store, | ||
DbMutationBatch.store, | ||
DbDocumentMutation.store, | ||
|
@@ -469,3 +566,10 @@ export const ALL_STORES = [ | |
DbTargetGlobal.store, | ||
DbTargetDocument.store | ||
]; | ||
|
||
/** | ||
* The list of all default IndexedDB stores used throughout the SDK. This is | ||
* used when creating transactions so that access across all stores is done | ||
* atomically. | ||
*/ | ||
export const ALL_STORES = [...V1_STORES]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is weird... I wonder if we should get rid of the V1_STORES list or else do:
(maybe V2_STORES is overkill, but I think we should at least have the comment explaining why we're using V1_STORES even though SCHEMA_VERSION is 2) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got rid of |
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.
Since you've reworked this so that the DBTargetGlobal always exists, I think we don't need to initialize metadata here, and I'd prefer not have these defaults duplicated unnecessarily between here and indexeddb_schema.ts. So perhaps change this to just:
private metadata: DbTargetGlobal = null;
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.
Done, and fixed the bug in the tests where we weren't calling
start()
but didn't know because we were initializing this to a dummy value.So, good suggestion!