Expose operation counts from Persistence layer #2164
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikelehen
approved these changes
Sep 9, 2019
There was a problem hiding this comment.
This looks like a reasonable port. But you seem ambivalent about if/how we should move forward with this. :-)
I don't have super strong feelings, but:
- I'm not wild about testing how code works (internal state) instead of just whether it works. It's easy for such tests to be fragile and break whenever the code changes, even if the code is still correct and working as intended.
- I would prefer if we could measure the benefit of index-free queries (and guard against regressions) via real perf tests, but we don't really have infrastructure for that. So maybe this is a reasonable alternative.
- I don't love having to pass statsCollectors into all of our persistence components... It's a little tempting to hang it off of the transaction or something.
- If we're looking for a reason not to move forward with this, I notice that it adds 2471 bytes to the SDK (533 after gzip) for test-only code.
Thoughts on how to proceed? If you want to check it in as-is, I'm fine with that. If we want to drop the PR, I'm probably fine with that too. But we should probably add a PORTING NOTE to Android or something to make it clear this is intentional.
|
Dropping in favor of more isolated approach in #2165 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a port of firebase/firebase-android-sdk#595
The ultimate goal of this PR is to allow us write unit tests that count how many items are processed during query execution. For this to work, this PR adds a StatsCollector. The RemoteDocumentCache and the MutationQueue have been updated to provide stats for every read, delete and insert operation. I tried to ensure that they counts between the different backing stores would match and added a unit test that ensures that this is true for a small subset of the operations.
I also changed the way that MemoryPersistence gets initialized to align it a bit closer with IndexedDbPersistence.
There are likely a bunch of issues with the stats accounting that we could either:
We could also drop this PR altogether and only do the counting on one platform.