-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix GeoIpDownloaderIT.testInvalidTimestamp
#137663
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Fixes a flaky test by removing the problematic assertion that assumed database files would be re-downloaded after resetting the validity setting. The test was failing because the GeoIP expiration logic deletes database chunks but doesn't re-download them when MD5 hashes remain unchanged, which is the case in the test fixture environment.
- Removed the second half of
testInvalidTimestampthat attempted to verify database re-download after resetting validity - Added a custom
IndexSettingProviderto disable request cache for the.geoip_databasesindex, ensuring cache doesn't mask the deletion of database chunks - Unmuted the test in
muted-tests.yml
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| muted-tests.yml | Removed test mute entry for testInvalidTimestamp |
| GeoIpDownloaderIT.java | Modified test to skip re-download verification, added cache-disabling plugin, and added assertion to verify chunk deletion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...est-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Pinging @elastic/es-data-management (Team:Data Management) |
szybia
left a comment
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
my impression is that this doesn't seem too pressing to test, as a reviewer i'd be digging more into why we can't change the hashes, but 🤷
The GeoIP expiration logic assumes that the md5 hashes of the databases change over time, as it deletes all database chunks from the .geoip_databases index. If the hashes do not change, no new database chunks are indexed and so the ingest nodes fail to download the databases as there are no chunks to download.
In this test suite, we don't have a way to change the database files served by the fixture, and thus change their md5 hashes.
Since we assume that in a real-world scenario the database files change over time, we are ok with skipping this part of the test.
See #128284 (comment) for a detailed investigation.
Closes #128284