Skip to content

Update the integration test to verify that bloom filter averted full requery #7095

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

Merged
merged 32 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
86870f9
update the test case to verify that the existence filter contains a b…
dconeybe Mar 7, 2023
f75534f
add retry
dconeybe Mar 7, 2023
dc42228
fix lint errors
dconeybe Mar 7, 2023
c1bff43
Update API reports
dconeybe Mar 7, 2023
1753549
testing_hooks.ts: add @internal tag to TestingHooks class so it doesn…
dconeybe Mar 7, 2023
50418ef
Add docs
dconeybe Mar 7, 2023
01b3bf0
fix lint errors
dconeybe Mar 7, 2023
384608c
cleanup
dconeybe Mar 7, 2023
67305ea
add a reference to b/271949433 in a comment
dconeybe Mar 7, 2023
f9e0707
get rid of unnecssary 'type IterationResult' declaration
dconeybe Mar 7, 2023
9bfdcc2
integration/firestore/gulpfile.js: add testing_hooks_util.ts to the l…
dconeybe Mar 7, 2023
15d47bc
testing_hooks_util.ts: REVERT ME: add _logWarn() to existenceFilterMi…
dconeybe Mar 7, 2023
739f13d
fix mangling of the property names of the argument to 'onExistenceFil…
dconeybe Mar 7, 2023
af53f1a
testing_hooks.ts: add @internal tag to ExistenceFilterMismatchInfo
dconeybe Mar 7, 2023
85ed7ab
avoid using objects for parameters since their names can get mangled …
dconeybe Mar 8, 2023
0b9aaf1
try hand-creating the ExistenceFilterMismatchInfo to see if it surviv…
dconeybe Mar 8, 2023
266a722
testing_hooks_util.ts: fix lint error
dconeybe Mar 8, 2023
c1437bc
firestore-compat: fix typing error in fields.test.ts: TS2339: Propert…
dconeybe Mar 8, 2023
24fbb79
Revert "firestore-compat: fix typing error in fields.test.ts: TS2339:…
dconeybe Mar 8, 2023
f9adb54
firestore-compat: *really* fix typing error in fields.test.ts: TS2339…
dconeybe Mar 8, 2023
def79d1
minor cleanups from self code review
dconeybe Mar 9, 2023
6d4e3dd
Merge remote-tracking branch 'origin/mila/BloomFilter' into BloomFilt…
dconeybe Mar 9, 2023
f5415f5
yarn prettier
dconeybe Mar 9, 2023
6bc7b3b
watch_change.ts minor cleanup
dconeybe Mar 9, 2023
bad0ca9
query.test.ts: remove it.only()
dconeybe Mar 9, 2023
9871614
refactor the test a bit
dconeybe Mar 9, 2023
61a9cd7
query.test.ts: it.only -> it (oops)
dconeybe Mar 9, 2023
2acf945
rename actualCount/expectedCount to localCacheCount/existenceFilterCo…
dconeybe Mar 9, 2023
daf049c
disable the annoying 'check changeset' check, which is broken when ta…
dconeybe Mar 9, 2023
74607ac
Merge remote-tracking branch 'origin/mila/BloomFilter' into BloomFilt…
dconeybe Mar 10, 2023
677b9e7
address review feedback
dconeybe Mar 10, 2023
acff0f4
yarn prettier
dconeybe Mar 10, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/check-changeset.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ name: Check Changeset

on:
pull_request:
branches-ignore:
- release
branches:
- master

env:
GITHUB_PULL_REQUEST_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
Expand Down
1 change: 1 addition & 0 deletions integration/firestore/gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function copyTests() {
testBase + '/integration/util/events_accumulator.ts',
testBase + '/integration/util/helpers.ts',
testBase + '/integration/util/settings.ts',
testBase + '/integration/util/testing_hooks_util.ts',
testBase + '/util/equality_matcher.ts',
testBase + '/util/promise.ts'
],
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,3 +196,4 @@ export type { ByteString as _ByteString } from './util/byte_string';
export { logWarn as _logWarn } from './util/log';
export { EmptyAuthCredentialsProvider as _EmptyAuthCredentialsProvider } from './api/credentials';
export { EmptyAppCheckTokenProvider as _EmptyAppCheckTokenProvider } from './api/credentials';
export { TestingHooks as _TestingHooks } from './util/testing_hooks';
34 changes: 34 additions & 0 deletions packages/firestore/src/remote/watch_change.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ import { logDebug, logWarn } from '../util/log';
import { primitiveComparator } from '../util/misc';
import { SortedMap } from '../util/sorted_map';
import { SortedSet } from '../util/sorted_set';
import {
ExistenceFilterMismatchInfo as TestingHooksExistenceFilterMismatchInfo,
TestingHooks
} from '../util/testing_hooks';

import { BloomFilter, BloomFilterError } from './bloom_filter';
import { ExistenceFilter } from './existence_filter';
Expand Down Expand Up @@ -445,6 +449,13 @@ export class WatchChangeAggregator {
purpose
);
}
TestingHooks.instance?.notifyOnExistenceFilterMismatch(
createExistenceFilterMismatchInfoForTestingHooks(
status,
currentSize,
watchChange.existenceFilter
)
);
}
}
}
Expand Down Expand Up @@ -815,3 +826,26 @@ function documentTargetMap(): SortedMap<DocumentKey, SortedSet<TargetId>> {
function snapshotChangesMap(): SortedMap<DocumentKey, ChangeType> {
return new SortedMap<DocumentKey, ChangeType>(DocumentKey.comparator);
}

function createExistenceFilterMismatchInfoForTestingHooks(
status: BloomFilterApplicationStatus,
localCacheCount: number,
existenceFilter: ExistenceFilter
): TestingHooksExistenceFilterMismatchInfo {
const result: TestingHooksExistenceFilterMismatchInfo = {
localCacheCount,
existenceFilterCount: existenceFilter.count
};

const unchangedNames = existenceFilter.unchangedNames;
if (unchangedNames) {
result.bloomFilter = {
applied: status === BloomFilterApplicationStatus.Success,
hashCount: unchangedNames?.hashCount ?? 0,
bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0,
padding: unchangedNames?.bits?.padding ?? 0
};
}

return result;
}
139 changes: 139 additions & 0 deletions packages/firestore/src/util/testing_hooks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/**
* @license
* Copyright 2023 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Manages "testing hooks", hooks into the internals of the SDK to verify
* internal state and events during integration tests. Do not use this class
* except for testing purposes.
*
* There are two ways to retrieve the global singleton instance of this class:
* 1. The `instance` property, which returns null if the global singleton
* instance has not been created. Use this property if the caller should
* "do nothing" if there are no testing hooks registered, such as when
* delivering an event to notify registered callbacks.
* 2. The `getOrCreateInstance()` method, which creates the global singleton
* instance if it has not been created. Use this method if the instance is
* needed to, for example, register a callback.
*
* @internal
*/
export class TestingHooks {
private readonly onExistenceFilterMismatchCallbacks = new Map<
Symbol,
ExistenceFilterMismatchCallback
>();

private constructor() {}

/**
* Returns the singleton instance of this class, or null if it has not been
* initialized.
*/
static get instance(): TestingHooks | null {
return gTestingHooksSingletonInstance;
}

/**
* Returns the singleton instance of this class, creating it if is has never
* been created before.
*/
static getOrCreateInstance(): TestingHooks {
if (gTestingHooksSingletonInstance === null) {
gTestingHooksSingletonInstance = new TestingHooks();
}
return gTestingHooksSingletonInstance;
}

/**
* Registers a callback to be notified when an existence filter mismatch
* occurs in the Watch listen stream.
*
* The relative order in which callbacks are notified is unspecified; do not
* rely on any particular ordering. If a given callback is registered multiple
* times then it will be notified multiple times, once per registration.
*
* @param callback the callback to invoke upon existence filter mismatch.
*
* @return a function that, when called, unregisters the given callback; only
* the first invocation of the returned function does anything; all subsequent
* invocations do nothing.
*/
onExistenceFilterMismatch(
callback: ExistenceFilterMismatchCallback
): () => void {
const key = Symbol();
this.onExistenceFilterMismatchCallbacks.set(key, callback);
return () => this.onExistenceFilterMismatchCallbacks.delete(key);
}

/**
* Invokes all currently-registered `onExistenceFilterMismatch` callbacks.
* @param info Information about the existence filter mismatch.
*/
notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void {
this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info));
}
}

/**
* Information about an existence filter mismatch, as specified to callbacks
* registered with `TestingUtils.onExistenceFilterMismatch()`.
*/
export interface ExistenceFilterMismatchInfo {
/** The number of documents that matched the query in the local cache. */
localCacheCount: number;

/**
* The number of documents that matched the query on the server, as specified
* in the ExistenceFilter message's `count` field.
*/
existenceFilterCount: number;

/**
* Information about the bloom filter provided by Watch in the ExistenceFilter
* message's `unchangedNames` field. If this property is omitted or undefined
* then that means that Watch did _not_ provide a bloom filter.
*/
bloomFilter?: {
/**
* Whether a full requery was averted by using the bloom filter. If false,
* then something happened, such as a false positive, to prevent using the
* bloom filter to avoid a full requery.
*/
applied: boolean;

/** The number of hash functions used in the bloom filter. */
hashCount: number;

/** The number of bytes in the bloom filter's bitmask. */
bitmapLength: number;

/** The number of bits of padding in the last byte of the bloom filter. */
padding: number;
};
}

/**
* The signature of callbacks registered with
* `TestingUtils.onExistenceFilterMismatch()`.
*/
export type ExistenceFilterMismatchCallback = (
info: ExistenceFilterMismatchInfo
) => void;

/** The global singleton instance of `TestingHooks`. */
let gTestingHooksSingletonInstance: TestingHooks | null = null;
Loading