Expose operation counts from Persistence layer #595
Conversation
| return batch; | ||
| } | ||
|
|
||
| @Nullable | ||
| @Override | ||
| public MutationBatch lookupMutationBatch(int batchId) { | ||
| statsCollector.recordRowsRead(MemoryMutationQueue.TAG, 1); |
There was a problem hiding this comment.
Can't this just be TAG? Also even if you don't want to name it just that, it's actually MutationQueue.TAG.
There was a problem hiding this comment.
Yeah, it should just be TAG. In an effort to keep it legible I renamed it to STATS_TAG.
| @@ -25,6 +25,9 @@ | |||
|
|
|||
| /** A queue of mutations to apply to the remote store. */ | |||
| interface MutationQueue { | |||
| /** The tag used by the StatsCollector. */ | |||
| String TAG = "Mutations"; | |||
There was a problem hiding this comment.
It would be nice if these tags matched the table names we're reading from. This one would be mutations and the other one remote_documents.
There was a problem hiding this comment.
Sounds good. Done.
| List<MutationBatch> result = new ArrayList<>(); | ||
| Set<Integer> uniqueBatchIds = new HashSet<>(); | ||
| while (longQuery.hasMoreSubqueries()) { | ||
| longQuery | ||
| .performNextSubquery() | ||
| .forEach( | ||
| row -> { | ||
| ++rowsRead[0]; |
There was a problem hiding this comment.
Would it make sense to just make this a part of forEach and similar methods (and the return value of the method)?
Just spitballing. Feel free to ignore.
There was a problem hiding this comment.
I like this idea and it's actually pretty simple. I'm just a little worried that this might cause the counts from Memory Persistence and SQLLite Persistence to differ (as we might have to skip rows in one implementation but not in the other), but we can cross that bridge when we get there.
| public SQLitePersistence( | ||
| Context context, | ||
| String persistenceKey, | ||
| DatabaseId databaseId, | ||
| LocalSerializer serializer, | ||
| StatsCollector statsProvider, |
There was a problem hiding this comment.
Your class is StatsCollector but the variable is statsProvider. Elsewhere you called it statsCollector too. Make these consistent?
There was a problem hiding this comment.
I renamed all statsProvider in this PR to statsCollector.
|
|
||
| SQLiteRemoteDocumentCache(SQLitePersistence persistence, LocalSerializer serializer) { | ||
| SQLiteRemoteDocumentCache( | ||
| SQLitePersistence persistence, LocalSerializer serializer, StatsCollector statsProvider) { |
There was a problem hiding this comment.
s/statsProvider/statsCollector/ as above
| */ | ||
| abstract class StatsCollector { | ||
| /** A stats collector that discards all operation counts. */ | ||
| static class NoOpStatsCollector extends StatsCollector { |
There was a problem hiding this comment.
This could be private static if newNoOpStatsCollector() had the return type of StatsCollector.
Alternatively, just consider making StatsCollector non-abstract and new StatsCollector() would be enough to get you a no-op instance. That would cut the line count here and reduce the number of classes in production by one.
There was a problem hiding this comment.
Are your really gonna make me change this PR from a "sophisticated stats framework" to a dumb counter? Okay then. It's done.
| @@ -849,17 +873,28 @@ public void testCanExecuteMixedCollectionQueries() { | |||
| allocateQuery(query); | |||
| assertTargetId(2); | |||
|
|
|||
| localStore.executeQuery(query); | |||
There was a problem hiding this comment.
This isn't awful but please consider making a separate test that specifically exercises interesting cases rather than making additional assertions as a side-effect of some other scenario. In the current configuration it becomes difficult to understand what part of the test is essential to verifying mixed collection queries vs what's essential to verifying the stats.
There was a problem hiding this comment.
I basically wanted to write the exact same test that we have here. I did remove the checks in 877 and 888, which should improve the readability of this test.
I can create a separate test but it would likely contain most lines from this test. What do you think?
There was a problem hiding this comment.
The worry I essentially have is that we end up wanting to change this test to verify some additional case and then get tripped up around the interaction with the counts and then it's not clear if changing the counts is the right thing to do.
I couldn't find a reference to it quickly, but some hardline testing guru types advocate for only having one assertion per test case essentially on these grounds.
Most of these tests are copies of each other in some way so I don't think that's much of an issue.
There was a problem hiding this comment.
Updated the PR to add a separate test.
|
/retest |
1 similar comment
|
/retest |
This PR adds a pretty-bare bones way to export document read counts from the RemoteDocumentCache and the MutationQueue. I will use this in more thorough tests once we start optimizing our query execution.
The different ways to construct the Persistence layer are getting a little out of hand. I think a good follow up would be to add Builders for some of the classes here and address the overload overload.