Reduce heap-memory usage of ingest-geoip plugin #28963
Conversation
With this commit we reduce heap usage of the ingest-geoip plugin by memory-mapping the database files. Previously, we have stored these files gzip-compressed but this has resulted that data are loaded on the heap. Closes elastic#28782
danielmitterdorfer
added
>enhancement
:Data Management/Ingest Node
|
Pinging @elastic/es-core-infra |
| @@ -80,28 +80,38 @@ | |||
| if (Files.exists(geoIpConfigDirectory) == false && Files.isDirectory(geoIpConfigDirectory)) { | |||
| throw new IllegalStateException("the geoip directory [" + geoIpConfigDirectory + "] containing databases doesn't exist"); | |||
| } | |||
|
|
|||
| boolean loadDatabaseOnHeap = Booleans.parseBoolean(System.getProperty("es.geoip.load_db_on_heap", "false")); | |||
There was a problem hiding this comment.
Do we really need this escape hatch? If this is to remain undocumented (which is should if it is kept as a sysprop), what would eventually allow us to remove this untested behavior?
There was a problem hiding this comment.
This is needed until we have a better understanding of the implications on nodes with small amounts of native memory.
| @@ -68,8 +67,7 @@ | |||
| private final Set<Property> properties; | |||
| private final boolean ignoreMissing; | |||
|
|
|||
| GeoIpProcessor(String tag, String field, DatabaseReader dbReader, String targetField, Set<Property> properties, | |||
| boolean ignoreMissing) throws IOException { | |||
There was a problem hiding this comment.
Thanks for removing unused exception.
|
I want to let this run through CI on master and will then backport accordingly later today. |
With this commit we reduce heap usage of the ingest-geoip plugin by memory-mapping the database files. Previously, we have stored these files gzip-compressed but this has resulted that data are loaded on the heap. Closes #28782
With this commit we reduce heap usage of the ingest-geoip plugin by memory-mapping the database files. Previously, we have stored these files gzip-compressed but this has resulted that data are loaded on the heap. Closes #28782
|
Backports: The backport to 5.6 is still pending. I want to discuss this with Martijn first because #27958 (added support for ASN) is missing as well. Depending on the result of the discussion I'd not backport at all to 5.6 or open a separate PR. I'll provide an update here after our discussion. |
|
I don't think this should have backported to 6.2, and I don't think it should be backported to 5.6. It is an enhancement more than it is a bug fix and we do not do those in patch releases. |
|
Ok, I agree. So let's not backport it to 5.6. Do you want me to revert the backport to 6.2? |
|
Please do. I would also add a migration note (it's not a breaking change) to the 6.3 migration docs if you could please. |
This reverts commit bf91962.
|
Reverted on 6.2 in 3a92555. I can also add a migration note later. |
This commit adds a note to the 6.3 docs that Geoip database files are now stored uncompressed. Relates elastic#28963
|
I've now opened #29006 to add migration docs for 6.3. |
This commit adds a note to the 6.3 docs that Geoip database files are now stored uncompressed. Relates #28963
With this commit we reduce heap usage of the ingest-geoip plugin by
memory-mapping the database files. Previously, we have stored these
files gzip-compressed but this has resulted that data are loaded on the
heap.
Closes #28782