Refactor geoip cache for MaxMind databases #125527
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CityResponse and EnterpriseResponse use the null-object pattern here, so postal is never null.
Allow a given lookup to create a cache-able record from the response from the database. At the moment every lookup just uses its response from the database as the cache-able record (this is fine).
so that it can be reused. Of course that means that we have to tweak all the existing callers to use method access rather than field access, hence the bigger diff than one might otherwise imagine.
joegallo
added
:Data Management/Ingest Node
|
Pinging @elastic/es-data-management (Team:Data Management) |
masseyke
reviewed
Mar 26, 2025
| } | ||
|
|
||
| @Override | ||
| protected AnonymousIpResponse record(AnonymousIpResponse response) { |
There was a problem hiding this comment.
I kind of wonder if this method name is going to cause confusion for future maintainers, since as you've pointed out your first thought when you see this is the verb -- what am I recording? What about cacheableRecord?
There was a problem hiding this comment.
Indeed, my unwavering love of short names is definitely at odds with the desire for a clear name in this case. I renamed it to cacheableRecord for now via 9dd8964, but I think I'm going to polish things a little more (on subsequent PRs).
There's too many nearby things named record or result or response. So here's a new word, 'entry'. A database has a bunch of entries in it.
💚 Backport successful
|
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 26, 2025
This was referenced Mar 27, 2025
joegallo
added a commit
to joegallo/elasticsearch
that referenced
this pull request
Mar 28, 2025
omricohenn
pushed a commit
to omricohenn/elasticsearch
that referenced
this pull request
Mar 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds another degree of freedom in the
MaxmindIpDataLookups. Rather than requiring that the response from theReaderis necessarily the same object as we keep in theGeoIpCache, it adds a newrecord(...)method (read as a noun, like "the record for storing in the cache") that allows a given MaxMindIpDataLookupto build a different object out of the response and then cache that object.In this PR, only the
CountryResponseis refactored to use this new capability -- it's the smallest of the response objects that would benefit from a change, so it's the easiest one to review at the same time as the rest of the machinery changes.We'll benefit from this new capability in two ways:
IngestDocumentfrom a record faster than we can update from a properResponse(as a simple example, the commongetName()method of various responses iterates over a collection -- now we'll do that just once when we populate the cache rather than every time we have a cache hit)Or, to put it simply, things are faster and use less memory this way.