Skip to content

Conversation

@martijnvg
Copy link
Member

This change helps facilitate allowing maxmind databases to be updated at runtime.
This will make is easier to purge the cache if a database changes.

Made the following changes:

  • Changed how geoip processor integrates with the cache. The cache is moved from the geoip processor to DatabaseReaderLazyLoader class and by doing so caching is not directly a concern to the geoip processor implementation. In a followup change the DatabaseReaderLazyLoader instance a processor is using will change after a database update.
  • Changed the cache key from ip + response class to ip + database_path.
  • Moved GeoIpCache from IngestGeoIpPlugin class to be a top level class.

This change helps facilitate allowing maxmind databases to be updated at runtime.
This will make is easier to purge the cache if a database changes.

Made the following changes:
* Changed how geoip processor integrates with the cache. The cache is moved from the geoip processor to DatabaseReaderLazyLoader class.
* Changed the cache key from ip + response class to ip + database_path.
* Moved GeoIpCache from IngestGeoIpPlugin class to be a top level class.
@martijnvg martijnvg added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.0.0 v7.12.0 labels Feb 5, 2021
@martijnvg martijnvg requested a review from probakowski February 5, 2021 12:26
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Feb 5, 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, thanks @martijnvg !
I left one comment about possible simplification

Comment on lines 136 to 176
CityResponse getCity(InetAddress ipAddress) {
SpecialPermission.check();
return AccessController.doPrivileged((PrivilegedAction<CityResponse>) () ->
cache.putIfAbsent(ipAddress, databasePath.toString(), ip -> {
try {
return get().city(ip);
} catch (AddressNotFoundException e) {
throw new GeoIpProcessor.AddressNotFoundRuntimeException(e);
} catch (Exception e) {
throw new RuntimeException(e);
}
}));
}

CountryResponse getCountry(InetAddress ipAddress) {
SpecialPermission.check();
return AccessController.doPrivileged((PrivilegedAction<CountryResponse>) () ->
cache.putIfAbsent(ipAddress, databasePath.toString(), ip -> {
try {
return get().country(ip);
} catch (AddressNotFoundException e) {
throw new GeoIpProcessor.AddressNotFoundRuntimeException(e);
} catch (Exception e) {
throw new RuntimeException(e);
}
}));
}

AsnResponse getAsn(InetAddress ipAddress) {
SpecialPermission.check();
return AccessController.doPrivileged((PrivilegedAction<AsnResponse>) () ->
cache.putIfAbsent(ipAddress, databasePath.toString(), ip -> {
try {
return get().asn(ip);
} catch (AddressNotFoundException e) {
throw new GeoIpProcessor.AddressNotFoundRuntimeException(e);
} catch (Exception e) {
throw new RuntimeException(e);
}
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe extract common functionality to separate method:

private <T extends AbstractResponse> T getResponse(InetAddress ipAddress, CheckedBiFunction<DatabaseReader, InetAddress, T,
    Exception> responseProvider) {
    SpecialPermission.check();
    return AccessController.doPrivileged((PrivilegedAction<T>) () ->
        cache.putIfAbsent(ipAddress, databasePath.toString(), ip -> {
            try {
                return responseProvider.apply(get(), ipAddress);
            } catch (AddressNotFoundException e) {
                throw new GeoIpProcessor.AddressNotFoundRuntimeException(e);
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        }));
}

CityResponse getCity(InetAddress ipAddress) {
    return getResponse(ipAddress, DatabaseReader::city);
}

CountryResponse getCountry(InetAddress ipAddress) {
    return getResponse(ipAddress, DatabaseReader::country);
}

AsnResponse getAsn(InetAddress ipAddress) {
    return getResponse(ipAddress, DatabaseReader::asn);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

@martijnvg martijnvg merged commit 5529b3d into elastic:master Feb 11, 2021
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Feb 11, 2021
Backport elastic#68581 of to 7.x branch.

This change helps facilitate allowing maxmind databases to be updated at runtime.
This will make is easier to purge the cache if a database changes.

Made the following changes:
* Changed how geoip processor integrates with the cache. The cache is moved from the geoip processor to DatabaseReaderLazyLoader class.
* Changed the cache key from ip + response class to ip + database_path.
* Moved GeoIpCache from IngestGeoIpPlugin class to be a top level class.
martijnvg added a commit that referenced this pull request Feb 11, 2021
Backport #68581 of to 7.x branch.

This change helps facilitate allowing maxmind databases to be updated at runtime.
This will make is easier to purge the cache if a database changes.

Made the following changes:
* Changed how geoip processor integrates with the cache. The cache is moved from the geoip processor to DatabaseReaderLazyLoader class.
* Changed the cache key from ip + response class to ip + database_path.
* Moved GeoIpCache from IngestGeoIpPlugin class to be a top level class.
@probakowski probakowski mentioned this pull request Feb 11, 2021
15 tasks
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 >non-issue Team:Data Management Meta label for data/management team v7.12.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants