Write pending queue during online indexing#4293
Conversation
jjezra
force-pushed
the
vector_index_pending_queue
branch
from
9b7b422 to
df51931
Compare
jjezra
force-pushed
the
vector_index_pending_queue
branch
3 times, most recently
from
1e62e12 to
bc84cd7
Compare
This indexing option will cause user updates to accumulate in a special queue during ongoing online indexing. The indexer will drain this queue after every transaction. This resolves 4292
| // Simulate a concurrent writer enqueueing new item while the drain is in progress: the index is still | ||
| // WRITE_ONLY_WITH_QUEUE, so this save is deferred back into the pending writes queue rather than indexed here. |
There was a problem hiding this comment.
Should this be done in a separate transaction?
| startBuildingSemaphore.acquire(); | ||
| startBuildingSemaphore.release(); | ||
|
|
||
| whilePaused.run(); |
There was a problem hiding this comment.
I think that is a good test to add, but I don't think it addresses the concern I was trying to point out, which is:
- the indexer indexes some records
- the indexer drains the queue
- something gets inserted into the queue
- the indexer indexes those records
Or:
- the indexer indexes some records
- something gets inserted into the queue
- the indexer drains the queue
- the indexer indexes those records
You are only testing one of those, but I'm not sure which.
Looking at the code, loading the config happens at the start of buildCommitRetryAsync, which means that our loop is always:
- the indexer loads the config (no-op on the first pass because of the semaphores)
- the indexer indexes some records
- the indexer drains the queue
- the indexer loads the config
- repeat
And we have no way to inject logic after the data is indexed, but before the queue is processed. I'm not sure I have a concrete issue that could arise there, but a general concern because we are missing that coverage.
| * Drain a pending writes queue. Used by the indexer. | ||
| */ | ||
| @ParametersAreNonnullByDefault | ||
| public class PendingWriteQueueDrainer { |
There was a problem hiding this comment.
It still seems valuable to test PendingWriteQueueDrainer in isolation. Or perhaps, given that PendingWritesQueueDrainerFactory now has a little more logic, having tests directly of that would make more sense. That might allow us to test some interactions that are hard as it is, since we would be able to do things like:
- call updateWhileWriteOnly on a record to insert it (simulating indexer)
- simultaneously update that record
- drain the queue into the index
The thing that I'm most concerned about, I think, is whether we can get in a state where:
- the record is indexed at v2
- the queue has an update from v1 -> v2
And whether that can cause problems.
Changing the constructor to not take an IndexingCommon, but instead take the 3 things it actually needs (assuming those are immutable in common), would decouple this and make it easier to test in isolation.
| /** | ||
| * Tests for building an index with a pending writes queue. | ||
| */ | ||
| class OnlineIndexerPendingWriteQueueTest extends OnlineIndexerTest { |
There was a problem hiding this comment.
- Lucene could be concerning, but we're planning to followup with reworking the storage, so that index maintainers would implement it themselves, meaning that Lucene would need tests when it starts supporting this
- concurrent index builders running concurrently (is this a legal case?) -- this is a legal case, primarily in mutual indexing. That being said, if you are enabling mutual indexing, you probably don't want the queue. But perhaps we should protect against that
- queue full -- IIUC this PR does not allow configuring the max size for a queue, so it would be hard to test filling the queue
- Multi-index seems valuable. It wouldn't be too hard to have a bug where all the records go in the same queue. This might not be super bad if they're on the same type, but if they're on different types, that would most likely result in corrupt indexes
- drain conflicts with concurrent save -- seems valuable, but should be well covered by the underlying queue, right? Or are you referring specifically to the markReadable code?
| /** | ||
| * Tests for building an index with a pending writes queue. | ||
| */ | ||
| class OnlineIndexerPendingWriteQueueTest extends OnlineIndexerTest { |
There was a problem hiding this comment.
We can discuss offline, but of #5 above - the intent is to have a test where the save fails because the drain "beat" it to the commit. This is specifically not covered by the underlying queue (there is a statement to that effect in the queue class) and is expected to be done by the markReadable.
The comment below (about testEnqueuedWriteFailsToCommitWhenIndexBecomesReadable seems to handle that.
| } | ||
|
|
||
| @Test | ||
| void writeOnlyWithQueueRoutesOutOfWindowUpdatesToQueue() throws Exception { |
There was a problem hiding this comment.
IIUC the concern was around how the index maintainer would behave if they were outside the window. The code here enqueues all of them, indicating that it doesn't even have the windowing information. That is probably sufficient, but it could mean that once the window is full by standard writes, it could decide to only enqueue entries that are within the window, which, I believe, would be wrong, or at least risky.
3 participants
During online indexing, under certain circumstances, short indexer transactions will repeatedly cause conflicts with user transactions (while the user attempts to update the index). To avoid this, we should add a new index state: WRITE_ONLY_WITH_QUEUE. In this index state, user updates will be added to a write-pending queue, and the indexer will drain the queue.