Enable GeoIP downloader by default #71505
Conversation
|
@elasticmachine run elasticsearch-ci/2 |
probakowski
added
:Data Management/Ingest Node
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-core-features (Team:Core/Features) |
...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java
Outdated
Show resolved
Hide resolved
| @@ -52,17 +52,11 @@ if (useFixture) { | |||
| } | |||
|
|
|||
| tasks.named("internalClusterTest").configure { | |||
| systemProperty "es.geoip_v2_feature_flag_enabled", "true" | |||
There was a problem hiding this comment.
In the toplevel build es.geoip_v2_disabled_in_tests is set to true, but I don't see it being set to false here.
How do the yaml, qa and java integ tests run with the geoip downloader enabled here?
There was a problem hiding this comment.
We switch geoip downloader on in tests themselves, at the beginning we assert state with it disabled then we start it and wait until its work is done. Earlier we had to switch flag because it controlled if downloader is registered at all, right now it only controls default value for enabled setting and everything is registered everytime.
There was a problem hiding this comment.
I see, I forgot about that. However I'm wondering whether for the geoip module, we can run all our tests without enabling the geoip downloader. It should by default be enabled and so there should be no need to enable it manually. I fear that that perhaps somewhere down the line we end up with an issue being reported by users that their databases aren't updated. If all our geoip integ tests enable the downloader if it is disabled, then we can never catch such a mistake.
So should we instead change the tests so that before each test is run we assert that the geoip downloader is enabled and unset the system property that toplevel build file is setting? (If that is the case, then we can address that in a followup pr.)
There was a problem hiding this comment.
Yeah, increased confidence in our default is worth changing it. I'd rather do it in followup though
|
@elasticmachine update branch |
This change enables GeoIP downloader by default. It removes feature flag but adds flag that is used by tests to disable it again (as we don't want to hammer GeoIP database service with every test cluster we spin up). Relates to elastic#68920
* Enable GeoIP downloader by default (#71505) This change enables GeoIP downloader by default. It removes feature flag but adds flag that is used by tests to disable it again (as we don't want to hammer GeoIP database service with every test cluster we spin up). Relates to #68920 * fix compilation * spotless * packaging tests * disableGeoIpDownloader * fix packaging
This change enables GeoIP downloader by default.
It removes feature flag but adds flag that is used by tests to disable it again (as we don't want to hammer GeoIP database service with every test cluster we spin up).
Relates to #68920