Add geo data cache

[heemin32]

Description

Add geo data cache

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.

[VijayanB]

Shouldn't this be singleton?

[heemin32]

That make sense.

[heemin32]

Cannot make it as a singleton as the maxSize property is coming from the caller.

[VijayanB]

Does it mean you will have multiple cache per cluster?

[heemin32]

There will be single cache per node. Any suggestion on how you want to make it as singleton? I would leave the code as it is and let the user of this class to decide. Because same can happen for many other classes as well which are singleton but does not have any protection on creating multiple instances.

[navneet1v]

Here is a suggestion to make class singleton per maxSize, We can use a factoryPattern to create a FactoryClass that provides GetDataCacheObjects for a specific size. If for an input size the object is already created we return the already created object otherwise we create an return the Object.

Also, we can make that GeoDataCache object creation is only accessible via the Factory class.

[heemin32]

That is not true singleton; having single instance for each size. Also, there is no use case where we want single instance for each cache size.

[heemin32]

Other option is making this as a private inner class of another class in which this cache is used that I can make in a next PR.

[codecov]

Codecov Report

Merging #340 (9b57b0f) into feature/ip2geo (eb5abf3) will decrease coverage by 0.25%.
The diff coverage is 80.00%.

❗ Current head 9b57b0f differs from pull request most recent head f788d37. Consider uploading reports for the commit f788d37 to get more accurate results

@@                 Coverage Diff                  @@
##             feature/ip2geo     #340      +/-   ##
====================================================
- Coverage             87.20%   86.95%   -0.25%     
+ Complexity              738      725      -13     
====================================================
  Files                    91       91              
  Lines                  2696     2660      -36     
  Branches                210      214       +4     
====================================================
- Hits                   2351     2313      -38     
- Misses                  261      264       +3     
+ Partials                 84       83       -1     

Impacted Files	Coverage Δ
...ensearch/geospatial/ip2geo/cache/GeoDataCache.java	80.00% <80.00%> (ø)

... and 14 files with indirect coverage changes

[navneet1v]

@heemin32 attach an issue with this PR.

Also add some information about this PR, what it contains

[heemin32]

@heemin32 attach an issue with this PR.

Also add some information about this PR, what it contains

There is no issue specific to this PR. I don't want to add the IP2Geo issue as there so many PR and it will spam the issue page. It contains a class implementing a cache...

[navneet1v]

Here is a suggestion to make class singleton per maxSize, We can use a factoryPattern to create a FactoryClass that provides GetDataCacheObjects for a specific size. If for an input size the object is already created we return the already created object otherwise we create an return the Object.

Also, we can make that GeoDataCache object creation is only accessible via the Factory class.

[navneet1v]

Couple of questions on this function:

1. can you provide some details why we need this function? Not able to think why we need such a function?
2. If the cache from where you are copying the data gets updated/deleted during the time of copying of data to new Cache object then the new cache will be stale.

[heemin32]

1. A user can change cache size dynamically through configuration change.
2. There is no public update/delete methods on cached value. Deletion can happen by automatic eviction by policy(cache size in this case). However, it is okay to keep the supposed to be deleted data a little longer.

[navneet1v]

This is an interesting condition. We are providing the update to max size, and if new size if less than the old size we are copying less data. This adds more doubt in my mind why we are even providing this functionality.

[heemin32]

This is not to throw away all the cached value when size is changed.

[navneet1v]

You have provided this very awesome object CacheKey, we should expose this class to other parts of the code so that cache key can be built and used directly for get and put function.
It will make the interface of get/put easy to use. Now exposing this has downside, lets say going forward if we want to update the cache key, in that case either we can create an internal CacheKey class that can be used, if we don't want all part of code start populating new fields in new cache key.

Given that thought process I think we should expose this CacheKey class to external parts of the code.

[heemin32]

I don't think so. There is no need for other class to know about it. There is no use case where caller need to construct the key itself. Actually, the interface of get is simpler by hiding the key.

[navneet1v]

we should catch this ExecutionException, and wrap it in another custom Exception and then throw it. We shouldn't expose the internals of Cache to outside parts of the code.

In my understanding GeoCache is just an abstraction over the OpenSearch Common Cache. Hence it make sense to avoid exposing internals out to other part of the code.

[heemin32]

ExecutionException is from java.util.concurrent I don't see a benefit of wrapping it into another exception.

[navneet1v]

why we are going with an internal OpenSearch cache implementation, when there are other tested and more prominent libraries for cache like Google Guava etc. Example: https://github.com/opensearch-project/k-NN/blob/8e2ad4595bd4f7c64d0b312cac30de8a42e4bf0b/src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java#L15-L14

Can we do some deep-dive here to figure out which one to choose and why?

[heemin32]

Current GeoIP processor use OpenSearch cache implementation which is well tested. Why do we want to explore new option here when there is working option already by adding additional dependency on third party library?