[Backport 10x] Move finishMerge to a finally block so it always runs (#14977) - #16481
[Backport 10x] Move finishMerge to a finally block so it always runs (#14977)#16481john-mlika wants to merge 3 commits into
Conversation
…mentMerger#cleanupMerge() (apache#16419)
github-actions
Bot
added
module:core/index
module:core/codecs
module:test-framework
labels
john-mlika
changed the title
msokolov
left a comment
msokolov
left a comment
There was a problem hiding this comment.
looks good; I just have a few small comments. Also there seems to be a merge conflict. Can you address these things? Then I'll merge this and put it on a list of bugfixes for a potential 10.5.1
| } | ||
|
|
||
| merge.mergeStartNS = System.nanoTime(); | ||
| merge.mergeStartNS = System.nanoTime(); |
There was a problem hiding this comment.
is there something funky with the indentation here?
There was a problem hiding this comment.
don't think so? the line just moved inside the try block
| } | ||
| } | ||
|
|
||
| private static Codec hnswFloatCodec() { |
There was a problem hiding this comment.
We have TestUtil.alwaysDocValuesFormat for this; I think you can use it instead?
There was a problem hiding this comment.
updated to use alwaysKnnVectorsFormat here which is the vector equivalent
|
|
||
| private static final int DIM = 16; | ||
|
|
||
| public void testSequentialAdviceIsRevertedAfterMerge() throws Exception { |
|
|
||
| public void testSequentialAdviceIsRevertedAfterMerge() throws Exception { | ||
| Recorder recorder = new Recorder(); | ||
| try (Directory raw = new ByteBuffersDirectory(); |
There was a problem hiding this comment.
IDK, should we use new MMapDirectory(createTempDir()) instead, just to more closely mirror the actual use case -- maybe someday somebody disables IOContext hinting on in-memory directories??
| for (int seg = 0; seg < 2; seg++) { | ||
| for (int i = 0; i < 64; i++) { | ||
| Document doc = new Document(); | ||
| float[] v = new float[DIM]; |
There was a problem hiding this comment.
Let's use BaseKnnVectorsFormatTestCase.randomNormalizedVector -- unless it's somehow not accessible here?
| List<String> sawSequential = new ArrayList<>(); | ||
| for (Map.Entry<String, List<String>> e : recorder.snapshot().entrySet()) { | ||
| String file = e.getKey(); | ||
| if (file.endsWith(".vec") == false) { |
There was a problem hiding this comment.
Lucene99FlatVectorsFormat.VECTOR_DATA_EXTENSION
|
|
||
| synchronized Map<String, List<String>> snapshot() { | ||
| Map<String, List<String>> copy = new LinkedHashMap<>(); | ||
| for (Map.Entry<String, List<String>> e : events.entrySet()) { |
There was a problem hiding this comment.
I'm not sure the snapshot method is really necessary since we don't expect any activity after the forceMerge? Could we return events directly? If we want to guard against any future modification, we could replace events with null, or an unmodifiable map.
There was a problem hiding this comment.
updated to return events directly and dropped the copy
|
I used the github webui to resolve the CHANGES.txt conflict -- but IDK where it puts that; you probably need to do the same on your PR branch |
- use MMapDirectory(createTempDir()) instead of ByteBuffersDirectory - use TestUtil#alwaysKnnVectorsFormat instead of a hand-rolled FilterCodec - use BaseKnnVectorsFormatTestCase#randomNormalizedVector for vector values - reference Lucene99FlatVectorsFormat#VECTOR_DATA_EXTENSION instead of .vec - drop the defensive copy in Recorder#snapshot, return the map directly
|
addressed the comments and resolved the conflict. should be good for another look now |
2 participants
Backport of #14977 to
branch_10x.#15732 removed the previous
KnnVectorsReader#finishMerge()caller on this branch, while #14977was never backported. Vector inputs can therefore retain
SEQUENTIALread advice after a merge.Details and reproduction are in #16419.
This restores
SegmentMerger#cleanupMerge()and its twoIndexWritercall sites, restores thestrict
AssertingKnnVectorsFormatinvariant, and adds a regression test. TheKnnVectorsWriterhunk from #14977 is omitted because it is already present here.