-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimize to fetch only fields relevant to doc level queries in doc level monitor instead of entire _source for each doc #1441
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,6 +236,28 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
} | ||
} | ||
|
||
val fieldsToBeQueried = mutableSetOf<String>() | ||
|
||
for (it in queries) { | ||
if (it.queryFieldNames.isEmpty()) { | ||
fieldsToBeQueried.clear() | ||
logger.debug( | ||
"Monitor ${monitor.id} : " + | ||
"Doc Level query ${it.id} : ${it.query} doesn't have queryFieldNames populated. " + | ||
"Cannot optimize monitor to fetch only query-relevant fields. " + | ||
"Querying entire doc source." | ||
) | ||
break | ||
} | ||
fieldsToBeQueried.addAll(it.queryFieldNames) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor - could put this an if with Unrelated to above - is there a scenario where we would want to turn this filtering off? Adding the setting to toggle on/off does no harm IMO, but not sure that it's necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retaining the feature flag as it's a super admin permission setting only so it can't be flipped off easily There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved logic for population of fieldsToBeQueried into if block |
||
if (monitorCtx.fetchOnlyQueryFieldNames && fieldsToBeQueried.isNotEmpty()) { | ||
logger.debug( | ||
"Monitor ${monitor.id} Querying only fields " + | ||
"${fieldsToBeQueried.joinToString()} instead of entire _source of documents" | ||
) | ||
} | ||
|
||
// Prepare DocumentExecutionContext for each index | ||
val docExecutionContext = DocumentExecutionContext(queries, indexLastRunContext, indexUpdatedRunContext) | ||
|
||
|
@@ -252,6 +274,7 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
docsToQueries, | ||
updatedIndexNames, | ||
concreteIndicesSeenSoFar, | ||
ArrayList(fieldsToBeQueried) | ||
) | ||
} | ||
} | ||
|
@@ -683,6 +706,7 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
docsToQueries: MutableMap<String, MutableList<String>>, | ||
monitorInputIndices: List<String>, | ||
concreteIndices: List<String>, | ||
fieldsToBeQueried: List<String>, | ||
) { | ||
val count: Int = docExecutionCtx.updatedLastRunContext["shards_count"] as Int | ||
for (i: Int in 0 until count) { | ||
|
@@ -697,8 +721,8 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
shard, | ||
prevSeqNo, | ||
maxSeqNo, | ||
null, | ||
docIds | ||
docIds, | ||
fieldsToBeQueried | ||
) | ||
val startTime = System.currentTimeMillis() | ||
transformedDocs.addAll( | ||
|
@@ -789,19 +813,15 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
shard: String, | ||
prevSeqNo: Long?, | ||
maxSeqNo: Long, | ||
query: String?, | ||
docIds: List<String>? = null, | ||
fieldsToFetch: List<String>, | ||
): SearchHits { | ||
if (prevSeqNo?.equals(maxSeqNo) == true && maxSeqNo != 0L) { | ||
return SearchHits.empty() | ||
} | ||
val boolQueryBuilder = BoolQueryBuilder() | ||
boolQueryBuilder.filter(QueryBuilders.rangeQuery("_seq_no").gt(prevSeqNo).lte(maxSeqNo)) | ||
|
||
if (query != null) { | ||
boolQueryBuilder.must(QueryBuilders.queryStringQuery(query)) | ||
} | ||
|
||
if (!docIds.isNullOrEmpty()) { | ||
boolQueryBuilder.filter(QueryBuilders.termsQuery("_id", docIds)) | ||
} | ||
|
@@ -816,6 +836,13 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
.size(10000) | ||
) | ||
.preference(Preference.PRIMARY_FIRST.type()) | ||
|
||
if (monitorCtx.fetchOnlyQueryFieldNames && fieldsToFetch.isNotEmpty()) { | ||
request.source().fetchSource(false) | ||
for (field in fieldsToFetch) { | ||
request.source().fetchField(field) | ||
} | ||
} | ||
val response: SearchResponse = monitorCtx.client!!.suspendUntil { monitorCtx.client!!.search(request, it) } | ||
if (response.status() !== RestStatus.OK) { | ||
logger.error("Failed search shard. Response: $response") | ||
|
@@ -906,7 +933,11 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
): List<Pair<String, TransformedDocDto>> { | ||
return hits.mapNotNull(fun(hit: SearchHit): Pair<String, TransformedDocDto>? { | ||
try { | ||
val sourceMap = hit.sourceAsMap | ||
val sourceMap = if (hit.hasSource()) { | ||
hit.sourceAsMap | ||
} else { | ||
constructSourceMapFromFieldsInHit(hit) | ||
} | ||
transformDocumentFieldNames( | ||
sourceMap, | ||
conflictingFields, | ||
|
@@ -927,6 +958,19 @@ class DocumentLevelMonitorRunner : MonitorRunner() { | |
}) | ||
} | ||
|
||
private fun constructSourceMapFromFieldsInHit(hit: SearchHit): MutableMap<String, Any> { | ||
if (hit.fields == null) | ||
return mutableMapOf() | ||
val sourceMap: MutableMap<String, Any> = mutableMapOf() | ||
for (field in hit.fields) { | ||
if (field.value.values != null && field.value.values.isNotEmpty()) | ||
if (field.value.values.size == 1) { | ||
sourceMap[field.key] = field.value.values[0] | ||
} else sourceMap[field.key] = field.value.values | ||
} | ||
return sourceMap | ||
} | ||
|
||
/** | ||
* Traverses document fields in leaves recursively and appends [fieldNameSuffixIndex] to field names with same names | ||
* but different mappings & [fieldNameSuffixPattern] to field names which have unique names. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we get an NPE here for migrations from older versions that did not have a queryFieldNames field in the DocLevelQuery objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor will initialize to empty list if not present in index.