-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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.totalcounter with file extensions.At the moment, we don't introspect into compound files, we might do it or not in the future.