Index-Free (4/6): Filter document queries by read time #617
Conversation
|
/retest |
schmidt-sebastian
changed the title
|
/test check-changed |
schmidt-sebastian
force-pushed
the
mrschmidt/indexfree-4
branch
from
156b2a0 to
bb50fc6
Compare
|
/test new-smoke-tests |
schmidt-sebastian
force-pushed
the
mrschmidt/indexfree-3
branch
from
d09d166 to
a97dc6e
Compare
|
Unassigning while I am waiting for PRs that this depends upon. |
schmidt-sebastian
force-pushed
the
mrschmidt/indexfree-4
branch
from
bb50fc6 to
1820b54
Compare
schmidt-sebastian
changed the base branch from
mrschmidt/indexfree-3
to
mrschmidt/indexfree-master
schmidt-sebastian
changed the title
| * Performs a query against the local view of all documents. | ||
| * | ||
| * @param query The query to match documents against. | ||
| * @param sinceUpdateTime If not set to SnapshotVersion.MIN, return only documents that have been |
There was a problem hiding this comment.
Oh. Yes. Fixed here and everywhere.
| @@ -112,6 +113,11 @@ public MaybeDocument get(DocumentKey key) { | |||
| continue; | |||
| } | |||
|
|
|||
| SnapshotVersion readTime = entry.getValue().second; | |||
| if (readTime.compareTo(sinceUpdateTime) <= 0) { | |||
There was a problem hiding this comment.
If there exist documents with a read time of zero and you pass SnapshotVersion.MIN, won't this skip them?
There was a problem hiding this comment.
It will, but SnapshotVersion.NONE is already special-cased in the client and usually indicates that we do not know the SnapshotVersion. Any document that gets written into the RemoteDocumentCache should have a known snapshot version - either the global snapshot version from Watch, or as the commit time from the WriteStream.
I added asserts to the code here to make sure that readTime is never zero, but this won't pass our tests until #695 lands.
There was a problem hiding this comment.
The migration in #686 didn't populate read time, so all existing documents in the cache will have a zero read time.
Could you add tests for how this is expected to work against pre-existing documents?
There was a problem hiding this comment.
We don't really have good testing infrastructure (yet) that tests client functionality after migration. We have some tests that pretend to do client like queries before and after a schema migration, but they are all low level. I'm not opposed to adding tests like this, but:
- the MemoryRemoteDocumentCache will not have any existing documents and is not affected by migrations
- the SQLiteRemoteDocumentCache reads all documents with NULL version numbers.
Let me know if you want me to add a test.
There was a problem hiding this comment.
At some level we should prove to ourselves that this works and that it's going to stay working. I see the NULL check on the read_time so I think you're right that this is functional.
On some level this could just be a test that validates the migration behaves as we expect. If it's straightforward to do so, it would be useful to stand up a SQLiteRemoteDocumentCache in the SQLiteSchemaTest and validate that migrated documents always match queries regardless of the update time you pass.
If you also deleted NoDocuments with a zero time in that migration you could validate that they don't exist post as well.
There was a problem hiding this comment.
Also, to be clear: If it costs too much time or effort to make it work I'm fine with skipping it but I think it's probably worth trying.
There was a problem hiding this comment.
I hacked together a test. It wasn't as bad as I thought it would be, but in return it is also not very pretty :) I am not yet validating NoDocument deletions (or deleting them for that matter).
schmidt-sebastian
force-pushed
the
mrschmidt/indexfree-4
branch
from
4b58f7f to
569f779
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/indexfree-4
branch
from
569f779 to
1e6792f
Compare
|
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This is the fourth of six PRs for "index-free" queries.
This PR adds the ability to
getAllDocumentsMatchingQuery()to skip documents older than a given update time. This allows us to only scan for documents that have been modified since the Query's Target Mapping has been persisted.