- Notifications
You must be signed in to change notification settings - Fork 25.5k
Add file extension metadata to cache miss counter from SharedBlobCacheService #134374
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
Conversation
Hi @tteofili, I've created a changelog YAML for you. |
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
…search into cachemiss_metric_filext
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.
LGTM
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.
Thanks Tommaso. This LGTM with one mention.
Also, should we open a linked serverless PR?
...ugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java Outdated Show resolved Hide resolved
@Nullable | ||
public static boolean isLuceneExtension(String ext) { | ||
return extensions.containsKey(ext); |
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.
nit: this is not nullable
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.
if I'm not mistaken, IndexFileNames#getExtension
might return null
in case the fileName
doesn't contain a .
, that's why I had put the @Nullable
annotation.
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.
Oh yes. I see what you mean.
We should @Nullable
the ext
method parameter ++
Being on the method it indicates the method can return null.
...ugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java Outdated Show resolved Hide resolved
return LuceneFilesExtensions.CFS.getExtension(); | ||
} | ||
try { | ||
String extension = IndexFileNames.getExtension(resourceDescription); |
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.
extension can be null here
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.
this is considered in the if statement
if (LuceneFilesExtensions.isLuceneExtension(extension)) ...
so a null
here will result in other
extension.
...ugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java Outdated Show resolved Hide resolved
* upstream/main: Add additional logging to make spotting stats issues easier (elastic#133972) [ESQL] Clean up ESQL enrich landing page (elastic#134820) ES|QL: Make kibana docs for Query settings more consistent (elastic#134881) Add file extension metadata to cache miss counter from SharedBlobCacheService (elastic#134374) Add IT for num_reduced_phases with batched query execution (elastic#134312) Remove `SizeValue` (elastic#134871)
This adds file extension metadata to cache miss counter when that's updated by
SharedBlobCacheService
.In particular this updates the
es.blob_cache.miss_that_triggered_read.total
counter with file extensions.At the moment, we don't introspect into compound files, we might do it or not in the future.