Add cache layer to reduce GeoIp data retrieval latency

[heemin32]

Description

Add cache layer to reduce GeoIp data retrieval latency

Issues Resolved

N/A

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Merging #343 (4147cef) into feature/ip2geo (eb5abf3) will increase coverage by 1.30%.
The diff coverage is 85.13%.

@@                 Coverage Diff                  @@
##             feature/ip2geo     #343      +/-   ##
====================================================
+ Coverage             87.20%   88.51%   +1.30%     
- Complexity              738      744       +6     
====================================================
  Files                    91       91              
  Lines                  2696     2681      -15     
  Branches                210      214       +4     
====================================================
+ Hits                   2351     2373      +22     
+ Misses                  261      230      -31     
+ Partials                 84       78       -6     

Impacted Files	Coverage Δ
...h/geospatial/ip2geo/processor/Ip2GeoProcessor.java	92.10% <75.00%> (+14.93%)	⬆️
...nsearch/geospatial/ip2geo/dao/Ip2GeoCachedDao.java	87.50% <83.33%> (-4.17%)	⬇️
...geospatial/ip2geo/action/GetDatasourceRequest.java	100.00% <100.00%> (+16.66%)	⬆️
...earch/geospatial/ip2geo/common/Ip2GeoSettings.java	92.85% <100.00%> (ø)
...opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java	87.20% <100.00%> (+6.18%)	⬆️
...opensearch/geospatial/plugin/GeospatialPlugin.java	90.69% <100.00%> (ø)

... and 5 files with indirect coverage changes

[vamshin]

why not 10k?

[heemin32]

This cache is created when Geospatial plugin is installed regardless a user uses Ip2Geo processor or not. So, I think it could be better to start with small number and let user to decide to increase the cache size later. Also, the performance test result with 1,000 wasn't really bad compared to 10,000.

[vamshin]

How do we make sure there is only one instance of GeoDataCache?

[heemin32]

If we create multiple Ip2GeoCachedDao, there will be more than on GeoDataCache. However, the class is used only for Ip2Geo processor. I didn't enforce the singleton of the cache as I thought the chance is low that we end up creating multiple Ip2GeoCachedDao by mistake.

We can enforce it to be singleton but not sure if it necessary at this stage.

public class GeoDataCache {
    private static GeoDataCache INSTANCE;
    public static GeoDataCache getInstance() {
        if (INSTANCE == null) {
            throw new IllegalStateException("instance is not initialized");
        }
        return INSTANCE;
    }

    public static void synchronized initialize(ClusterStateService clusterService) {
        if (INSTANCE != null) {
            throw new IllegalStateException("instance is initialized already");
        }
        INSTANCE = new GeoDataCache(clusterService.getClusterSettings().get(Ip2GeoSettings.CACHE_SIZE));
        clusterService.getClusterSettings()
            .addSettingsUpdateConsumer(Ip2GeoSettings.CACHE_SIZE, setting -> INSTANCE.updateMaxSize(setting.longValue()));
    }

    ....
}

Usage

    GeoDataCache.initialize(clusterService);
    GeoDataCache geoDataCache = GeoDataCache.getInstance();

[martin-gaievski]

maybe add a code comment explaining this reasoning

[martin-gaievski]

please make timeout value a constant

[heemin32]

Good catch. Missed that.

[martin-gaievski]

maybe add a code comment explaining this reasoning

[martin-gaievski]

I've seen in core such randomized test cases they do in multiple runs, so chance is high that during one run variable will take all possible values. Like a simple for loop with 5 to 10 iterations.

[vamshin]

LGTM! Thanks