Use sequence numbers instead of read time in collection group #3111
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
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (e4ee5a67) is created by Prow via merging commits: 4e336c6 1a5aaa9. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (027297d5) is created by Prow via merging commits: 9e93fff 7e6b736. |
There was a problem hiding this comment.
I think this PR needs to wait until we have a holistic view on how we keep track of mutation batches and partial updates (e.g. we processed 10 out of 100 documents for a single read time). I am no longer certain that sequence numbers are the right solution here, so it would be good to confirm that they are before we proceed.
| String getNextCollectionGroupToUpdate(int currentMaxSequenceNumber); | ||
|
|
||
| /** Returns the highest sequence number in the collection groups table. */ | ||
| int getMaxCollectionGroupSequenceNumber(); |
There was a problem hiding this comment.
getIndexBackfillSequenceNumber? I think it might be more interesting to say what the sequence number is for.
I am 50/50 on whether we need this method in our interface though.
There was a problem hiding this comment.
Not sure what to name it. The Index Manager is just getting the highest sequence number, but the IndexBackfiller is assigning it meaning by storing the most recently written collection group to it.
I think it's best to leave it in the manager since the IndexBackfiller currently delegates all SQL calls to the index manager.
|
|
||
| // TODO(indexing): Handle pausing and resuming from the correct document if backfilling hits the | ||
| // max doc limit while processing docs for a certain read time. |
There was a problem hiding this comment.
That's a very big todo :)
There was a problem hiding this comment.
Just backfilling it in from before :)
Will tackle this in a subsequent PR
| @@ -55,6 +54,7 @@ | |||
| /** A persisted implementation of IndexManager. */ | |||
| final class SQLiteIndexManager implements IndexManager { | |||
| private static final String TAG = SQLiteIndexManager.class.getSimpleName(); | |||
| private static final int NEW_COLLECTION_GROUP_SEQUENCE_NUMBER = 0; | |||
There was a problem hiding this comment.
Should we use -1 here? 0 poses the question whether we have indexed the first entry (with a zero-based index).
| // TODO(indexing): Store progress with a counter rather than a timestamp. If using a timestamp, | ||
| // use the read time of the last document read in the loop. | ||
| setCollectionGroupUpdateTime(nextCollectionGroup[0], Timestamp.now()); | ||
| setCollectionGroup(nextCollectionGroup[0], getMaxCollectionGroupSequenceNumber() + 1); |
There was a problem hiding this comment.
Hm, this should only be set after we are done with the update.
There was a problem hiding this comment.
Done. Also refactored the code so that the IndexBackfiller does not have to deal with sequence numbers.
thebrianchen
commented
Nov 19, 2021
There was a problem hiding this comment.
Made some revisions:
- Mark collection group as completed only after indexing all documents in the collection.
- Removed
getMaxCollectionGroupSequenceNumber()from index manager and instead let the backfiller track when all collection groups have been processed. - Added test to verify that collection groups that are partially indexed are processed first.
|
|
||
| // TODO(indexing): Handle pausing and resuming from the correct document if backfilling hits the | ||
| // max doc limit while processing docs for a certain read time. |
There was a problem hiding this comment.
Just backfilling it in from before :)
Will tackle this in a subsequent PR
| String getNextCollectionGroupToUpdate(int currentMaxSequenceNumber); | ||
|
|
||
| /** Returns the highest sequence number in the collection groups table. */ | ||
| int getMaxCollectionGroupSequenceNumber(); |
There was a problem hiding this comment.
Not sure what to name it. The Index Manager is just getting the highest sequence number, but the IndexBackfiller is assigning it meaning by storing the most recently written collection group to it.
I think it's best to leave it in the manager since the IndexBackfiller currently delegates all SQL calls to the index manager.
| // TODO(indexing): Store progress with a counter rather than a timestamp. If using a timestamp, | ||
| // use the read time of the last document read in the loop. | ||
| setCollectionGroupUpdateTime(nextCollectionGroup[0], Timestamp.now()); | ||
| setCollectionGroup(nextCollectionGroup[0], getMaxCollectionGroupSequenceNumber() + 1); |
There was a problem hiding this comment.
Done. Also refactored the code so that the IndexBackfiller does not have to deal with sequence numbers.
| // Mark the collection group as fully indexed if all documents in the collection have been | ||
| // indexed during this backfill iteration. | ||
| if (documentsRemaining >= documents.size()) { | ||
| indexManager.markCollectionGroupIndexed(collectionGroup); |
There was a problem hiding this comment.
This is different from how I implemented it. I always increment the sequence number regardless of whether I am done or not - it's a tiny bit simpler but may not be as optimal as what you are doing. We should discuss.
|
@thebrianchen: The following tests 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. |
|
Closing the PR since this was incorporated into #3153 |
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.
Instead of using read times to track the ordering of collection groups in the
collection_group_update_timestable, use a sequence counter instead.Looking for naming suggestions: