Skip to content

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

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.

Sign up for GitHub

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

Merged
martijnvg merged 1 commit into from
Mar 10, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.elasticsearch.search.sort.SortOrder;
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
import org.elasticsearch.test.ESIntegTestCase.Scope;
import org.elasticsearch.test.junit.annotations.TestLogging;
import org.junit.After;

import java.io.BufferedOutputStream;
Expand Down Expand Up @@ -143,7 +144,7 @@ public void testGeoIpDatabasesDownload() throws Exception {
}
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/69972")
@TestLogging(value = "org.elasticsearch.ingest.geoip:TRACE", reason = "https://github.com/elastic/elasticsearch/issues/69972")
public void testUseGeoIpProcessorWithDownloadedDBs() throws Exception {
// setup:
BytesReference bytes;
Expand Down
17 changes: 15 additions & 2 deletions modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,19 @@ void retrieveAndUpdateDatabase(String databaseName, GeoIpTaskState.Metadata meta
return;
}

// 2 types of threads:
// 1) The thread that checks whether database should be retrieved / updated and creates (^) tmp file (cluster state applied thread)
// 2) the thread that downloads the db file, updates the databases map and then removes the tmp file
// Thread 2 may have updated the databases map after thread 1 detects that there is no entry (or md5 mismatch) for a database.
// If thread 2 then also removes the tmp file before thread 1 attempts to create it then we're about to retrieve the same database
// twice. This check is here to avoid this:
DatabaseReaderLazyLoader lazyLoader = databases.get(databaseName);
if (lazyLoader != null && recordedMd5.equals(lazyLoader.getMd5())) {
LOGGER.debug("deleting tmp file because database [{}] has already been updated.", databaseName);
Files.delete(databaseTmpGzFile);
return;
}

final Path databaseTmpFile = Files.createFile(geoipTmpDirectory.resolve(databaseName + ".tmp"));
LOGGER.info("downloading geoip database [{}] to [{}]", databaseName, databaseTmpGzFile);
retrieveDatabase(
Expand All @@ -250,8 +263,8 @@ void retrieveAndUpdateDatabase(String databaseName, GeoIpTaskState.Metadata meta
failure -> {
LOGGER.error((Supplier<?>) () -> new ParameterizedMessage("failed to download database [{}]", databaseName), failure);
try {
Files.delete(databaseTmpFile);
Files.delete(databaseTmpGzFile);
Files.deleteIfExists(databaseTmpFile);
Files.deleteIfExists(databaseTmpGzFile);
} catch (IOException ioe) {
ioe.addSuppressed(failure);
LOGGER.error("Unable to delete tmp database file after failure", ioe);
Expand Down