Limit num hits when reading Lucene changes #30908
Conversation
Today we don't limit the number of hits when reading changes from Lucene index. If the index and the requesting seq# range both are large, the searcher may consume a huge amount of memory. This commit uses a fixed size batch with search_after to avoid the problem.
dnhatn
added
>feature
:Distributed Indexing/Engine
|
Pinging @elastic/es-distributed |
bleskes
left a comment
bleskes
left a comment
There was a problem hiding this comment.
I left a question. I was also wondering if we have already a test in place which makes sure that we overflow the SEARCH_BATCH_SIZE (as I don't see tests here)?
| .add(LongPoint.newRangeQuery(SeqNoFieldMapper.NAME, fromSeqNo, toSeqNo), BooleanClause.Occur.FILTER) | ||
| .build(); | ||
| private TopDocs searchOperations(ScoreDoc after) throws IOException { | ||
| final Query rangeQuery = LongPoint.newRangeQuery(SeqNoFieldMapper.NAME, fromSeqNo, toSeqNo); |
There was a problem hiding this comment.
why did we drop the DocValuesFieldExistsQuery(SeqNoFieldMapper.PRIMARY_TERM_NAME) part?
There was a problem hiding this comment.
DocValuesFieldExistsQuery(SeqNoFieldMapper.PRIMARY_TERM_NAME) was in the temporary fix (7004d2c). This clause was to use to eliminate nested docs.
| * A {@link Translog.Snapshot} from changes in a Lucene index | ||
| */ | ||
| final class LuceneChangesSnapshot implements Translog.Snapshot { | ||
| static final int SEARCH_BATCH_SIZE = 100; |
There was a problem hiding this comment.
Can this be specified as a parameter via newLuceneChangesSnapshot(...) method instead of a constant? Then in ccr we can make this configurable.
Also 100 feels on the low side to me. Maybe default to 1024?
|
@bleskes and @martijnvg I've added a test that verifies reading multiple batches. Could you please have another look? Thank you! |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
Left one comment. Otherwise LGTM
| Searcher searcher = acquireSearcher(source, SearcherScope.INTERNAL); | ||
| try { | ||
| LuceneChangesSnapshot snapshot = new LuceneChangesSnapshot(searcher, mapperService, minSeqNo, maxSeqNo, requiredFullRange); | ||
| final int batchSize = preferredSearchBatchSize <= 0 ? Engine.LUCENE_HISTORY_DEFAULT_SEARCH_BATCH_SIZE : preferredSearchBatchSize; |
There was a problem hiding this comment.
I think it is better to define this default just in ccr (ShardChanges.java)?
(and let newLuceneChangesSnapshot(...) methods just not allow values lower than 1)
There was a problem hiding this comment.
If we do this (and I'm not convinced we should) that we please not use a magic number for the parameter? people should just say what they want or use this constant directly
bleskes
left a comment
bleskes
left a comment
There was a problem hiding this comment.
I left some more minor comments and one question I will reach out to discuss.
| Searcher searcher = acquireSearcher(source, SearcherScope.INTERNAL); | ||
| try { | ||
| LuceneChangesSnapshot snapshot = new LuceneChangesSnapshot(searcher, mapperService, minSeqNo, maxSeqNo, requiredFullRange); | ||
| final int batchSize = preferredSearchBatchSize <= 0 ? Engine.LUCENE_HISTORY_DEFAULT_SEARCH_BATCH_SIZE : preferredSearchBatchSize; |
There was a problem hiding this comment.
If we do this (and I'm not convinced we should) that we please not use a magic number for the parameter? people should just say what they want or use this constant directly
| public static final String SYNC_COMMIT_ID = "sync_id"; | ||
| public static final String HISTORY_UUID_KEY = "history_uuid"; | ||
| // The default number of hits that one search should return when reading Lucene history | ||
| public static final int LUCENE_HISTORY_DEFAULT_SEARCH_BATCH_SIZE = 1024; |
There was a problem hiding this comment.
can you move this to LuceneChangesSnapshot ? this will allow using a short name like DEFAULT_BATCH_SIZE
| return operations; | ||
| } | ||
|
|
||
| private int randomSearchBatchSize() { |
There was a problem hiding this comment.
we only need a method for this because the default is long. Can we use a short default name or a constant and just inline this.
| } | ||
| } | ||
|
|
||
| public void testSearchMultipleBatches() throws Exception { |
There was a problem hiding this comment.
sine we now randomize the batching in tests, i don't think we need this dedicated test any more?
|
I discussed this with @martijnvg and agreed to back out the extra parameter. @bleskes Could you please take a look? Thank you! |
|
@elasticmachine test this please |
|
Thanks everyone! |
Today we don't limit the number of hits when reading changes from Lucene index. If the index and the requesting seq# range both are large, the searcher may consume a huge amount of memory. This commit uses a fixed size batch with search_after to avoid the problem.
5 participants
Today we don't limit the number of hits when reading changes from Lucene
index. If the index and the requesting seq# range both are large, the
searcher may consume a huge amount of memory.
This commit uses a fixed size batch with search_after to avoid the
problem.