Handle multiple updates to the same record in a transaction

[ohadzeliger]

The issue at hand is that when running multiple update operations in a single transaction, the partition's document counts and the PK-segment index may get into an inconsistent state. The root cause is that the first update in the transaction clears the doc from the Lucene index and the PK index. Since the changes are not flushed, the IndexWriter has them cached in the NRT cache. The second record update would then not find the record in the PK index (because the segment has changed but the IndexReader does not yet reflect that) and therefore the delete is skipped, including updating the partition count. Note that it does attempt a delete-by-query that actually removes the doc from the Lucene index, but since we can't know that, the partition is not updated.
The solution is to refresh the DirectoryReader when doing an update, so that any previously written changes are showing up. The refresh operation uses DirectoryReader.openIfChanged that is more efficient in resources than using a brand new open call.

Resolve #3704

[jjezra]

Suggested change

	* This method would first try to find the document using teh existing reader. If it can't, it will refresh the reader
	* This method would first try to find the document using the existing reader. If it can't, it will refresh the reader

[ohadzeliger]

Done.

[jjezra]

Please consider rewording.

[ohadzeliger]

Done.

[ScottDugas]

What happens if this is touched concurrently?

[ohadzeliger]

Added test to update the readers concurrently

[ScottDugas]

I still feel like there is a race condition, but it's possible that it doesn't work out in practice.

We start with writerReader0

1. threadA calls openIfChanged resulting in newReader1
2. threadB changes the writer -- This may not be possible to do without also creating a new writerReader, given the way this method is used, your test, I believe does call getWriterReader to be called when it saves.
3. threadC calls openIfChanged resulting in newReader2.
4. threadA adds writerReader0 to readersToClose
5. threadC adds writerReader0 to readersToClose
6. threadA sets writerReader to writerReader1 and returns it
7. threadC sets writerReader to writerRearedr2 and returns it

In this case, none of the code here will close writerReader1. So it might be worthwhile to change it so that we add the created reader to readersToClose, and we don't close writerReader directly.
i.e.
in the constructor call:
readersToClose(writerReader)
and do so again here, with a local:

newWriterReader = LazyCloseable.supply(() -> newReader);
readersToClose.add(newWriterReader);
writerReader = newWriterReader

[ScottDugas]

Also, do you need to set writerReader to volatile?

[ohadzeliger]

Yeah, I think there is a possibility of a race condition. I believe you only need two threads making a change to the writer to expose this.

[ohadzeliger]

Done.
I don't think it is necessary to add volatile to the writerReader field, as in each thread, if the delete failed and the thread has caused a new reader to be created, this new reader should have the changed document cached, and if it is not, it will not be available in other threads, so a cached value in a thread should be OK in that situation.