Skip to content
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

Fix DatabaseRegistryTests #70180

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

martijnvg
Copy link
Member

This test predefines expected md5 hashes in constants, that were expected with java15.
However java16 creates different md5 hashes and so the expected md5 hashes don't match
with the actual md5 hashes, which caused tests in this test suite to fail (running
with java16 only).

The tests now generates the expected md5 hash during the test instead of using predefined constants.

Closes #69986

This test predefined expected md5 hashes in constants, that were expected with java15.
However java16 creates different md5 hashes and so the expected md5 hashes don't match
with the actual md5 hashes, which caused tests in this test suite to fail (running
with java16 only).

The tests now generates the expected md5 hash during the test instead of using predefined constants.

Closes elastic#69986
@martijnvg martijnvg added >test Issues or PRs that are addressing/adding tests :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.0.0 labels Mar 9, 2021
@martijnvg martijnvg requested a review from probakowski March 9, 2021 20:30
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Mar 9, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (Team:Core/Features)

Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍
Worth noting that hashes are different because gzip header changed in JDK 16 (JDK-8244706). This problem can also be fixed by adding all[9] = 0; in gzip method (DatabaseRegistryTests:316) but calculating hash every time is probably more future-proof

@martijnvg
Copy link
Member Author

Worth noting that hashes are different because gzip header changed in JDK 16 (JDK-8244706).

Thanks for figuring this out. I was unable to pin point what changed in jdk16, but sure that something did change.

but calculating hash every time is probably more future-proof

Right, that is why I went ahead with this test change.

@martijnvg martijnvg merged commit 0c82c4c into elastic:master Mar 10, 2021
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Mar 10, 2021
This test predefined expected md5 hashes in constants, that were expected with java15.
However java16 creates different md5 hashes and so the expected md5 hashes don't match
with the actual md5 hashes, which caused tests in this test suite to fail (running
with java16 only).

The tests now generates the expected md5 hash during the test instead of using predefined constants.

Closes elastic#69986
martijnvg added a commit that referenced this pull request Mar 10, 2021
…ownloader (#69971)

Backport of #69540 to 7.x branch.

This component is responsible for making the databases maintained by GeoIpDownloader
available for ingest processors.

Also provided a lookup mechanism for geoip processors with fallback to {@link LocalDatabases}.
All databases are downloaded into a geoip tmp directory, which is created at node startup.

The following high level steps are executed after each cluster state update:
1) Check which databases are available in {@link GeoIpTaskState},
   which is part of the geoip downloader persistent task.
2) For each database check whether the databases have changed
   by comparing the local and remote md5 hash or are locally missing.
3) For each database identified in step 2 start downloading the database
   chunks. Each chunks is appended to a tmp file (inside geoip tmp dir) and
   after all chunks have been downloaded, the database is uncompressed and
   renamed to the final filename. After this the database is loaded and
   if there is an old instance of this database then that is closed.
4) Cleanup locally loaded databases that are no longer mentioned in {@link GeoIpTaskState}.

Relates to #68920

Other cherry-picked commits:

* Fix ReloadingDatabasesWhilePerformingGeoLookupsIT (#70163)

Wait for ingest threads to stop using the DatabaseReaderLazyLoader, so the during the next run the db update thread doesn't try to remove the db again (because the file hasn't yet been deleted).

Also delete tmp dirs this test create at the end of the test, so that when repeating this test many times, this test doesn't accumulate many directories with database files.

Closes #69980

* Fix clean up of old entries in DatabaseRegistry.initialize (#70135)

This change switches clean up in DatabaseRegistry.initialize from using Files.walk and stream operations to Files.walkFileTree which can be made more robust in case of errors

* Fix DatabaseRegistryTests (#70180)

This test predefined expected md5 hashes in constants, that were expected with java15.
However java16 creates different md5 hashes and so the expected md5 hashes don't match
with the actual md5 hashes, which caused tests in this test suite to fail (running
with java16 only).

The tests now generates the expected md5 hash during the test instead of using predefined constants.

Closes #69986

* Fix GeoIpDownloaderIT#testUseGeoIpProcessorWithDownloadedDBs(...) test (#70215)

The test failure looks legit, because there is a possibility that the same databases
was downloaded twice. See added comment in DatabaseRegistry class.

Relates to #69972

* Muted GeoIpDownloaderIT#testUseGeoIpProcessorWithDownloadedDBs(...) test,
see #69972

Co-authored-by: Przemko Robakowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatabaseRegistryTests failing
4 participants