[Feature] New module geospatial-client for cross-plugin IP enrichment

[andy-k-improving]

Description

This is an implementation ticket which follow the discussion of #698, to create a new Gradle module for other OpenSearch Plugin to import, in order to make an IP enrichment call to GeoSpatial plugin to resolve GeoLocation data with existing data source.

Hight-level code changes:

• A new Gradle module geospatial-client with appreciate Gradle setting files to publish it as a standalone Jar.
• A core class ipEnrichmentActionClient which serve as the interface, this is the only way for other Plugin to interact with GeoSpatial's IP enrichment functionality
• A collection of classes to support the IP enrichment remote call access IpEnrichmentAction, IpEnrichmentRequest and IpEnrichmentResponse
• IpEnrichementTransportAction class which contain the logic for the IP address lookup and register it during the GeoSpatial's bootstrap time

Non GeoSpatial plugin can now interact with the new functionality like below:

IpEnrichmentActionClient ipClient = new IpEnrichmentActionClient(client.getNodeClient());
        try {
          Map<String, Object> geoLocationData = ipClient.getGeoLocationData("50.68.18.229");
        } catch (Exception e) {
            throw new RuntimeException(e);
        }

Related Issues

Resolves #698

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[andy-k-improving]

@heemin32 here is the implementation for #698 as we discussed earlier of the week, would you mind to have a look and advise?
Any comment will be appreciated.

[navneet1v]

Looked very high level on the code. I like the idea of having a client. But I want to get some consistencies in the code. and I am hoping client to very lightweight.

[navneet1v]

For all the tests lets use the same test framework used in plugin rather than using standard Mockito ensure that test framework is same across different modules of the plugin.

[andy-k-improving]

I have updated the build.gradle to read the Mockito version from the opensearch.build plugin, in this case we will have the same version of Mockito as the main OpenSearch repo.

testImplementation "org.mockito:mockito-core:${versions.mockito}"

Or you mean I should ditch Mockito all together?

[navneet1v]

My idea was tests in this client should extend from OpenSearchTestCase just like the plugin and not specifically on the mockito version.

[heemin32]

It seems we don't extend from OpenSearchTestCase for client module? https://github.com/opensearch-project/ml-commons/blob/main/client/src/test/java/org/opensearch/ml/client/MachineLearningNodeClientTest.java

[navneet1v]

The problem with not doing this would be that we first we will not able to test the code with random data and another problem is since we will not have seed whenever we want to reproduce a failure. Not sure why MLCommons didn't do this. This is an acceptable practice in Opensearch Plugins. Plus you will miss out on all the different mocking and testing framework added by Opensearch.

[andy-k-improving]

In my point, this depends on the amount of logic to test.
I agree we should have it to remain the consistency, but for now if we aim to have it as a thin client which has no actual logic inside except the argument validation.
This may be a overkill.

[andy-k-improving]

@navneet1v I have fixed most of the point except the Mockito, can you have a look on the review?
Thanks,

[andy-k-improving]

@heemin32 I updated the code to reflect comments, can you have another look.
Also whats your thought on the cluster upgrade scenario?

[heemin32]

@heemin32 I updated the code to reflect comments, can you have another look. Also whats your thought on the cluster upgrade scenario?

I think, we can add versioning on this serialization/deserialization using OpenSearch version?

[heemin32]

@Xtansia Could you take a look at this?

[andy-k-improving]

I think, we can add versioning on this serialization/deserialization using OpenSearch vers

Just to align on this, what's the expected behaviour in the case when serializationID doesn't match?
Should we thrown an exception, or we should attempt to parse it as per the serializationID? (Ex: v1.19.0 version of the plugin serializating the object from v1.18.0...etc)

[andy-k-improving]

@heemin32 I updated the code to reflect comments, can you have another look. Also whats your thought on the cluster upgrade scenario?

I think, we can add versioning on this serialization/deserialization using OpenSearch version?

I have updated the code to perform version check, can you have a look and advise?
Thx!

[heemin32]

Another general comment is if we should implement it as a rest API rather than client? What do you think? @andy-k-improving

[andy-k-improving]

Another general comment is if we should implement it as a rest API rather than client? What do you think? @andy-k-improving

We should have it in the long term || as day-2, but I don't think that make sense to bundle it in one PR, as there will be more considerations I foresee (Ex: performance, web-layer security) need to be addressed || discussed ahead of the implementation.
Let me create another issue perhaps?

[andy-k-improving]

@heemin32 Also do you foresee any problem to publish the geospatial-client jar?
In local, I see the jar is produced correctly, but not sure will that be the same when publishing to AWS || Maven central.

[heemin32]

Let me create another issue perhaps?

No need for that. I was considering either this or that, but not both. Thanks.

[heemin32]

@heemin32 Also do you foresee any problem to publish the geospatial-client jar? In local, I see the jar is produced correctly, but not sure will that be the same when publishing to AWS || Maven central.

I don’t have much expertise on that topic. @ylwu-amzn, is there anything specific we should consider when publishing the client? Asking as ml-common already has done it.

[andy-k-improving]

No need for that. I was considering either this or that, but not both. Thanks.

If that is the case, then I rather keep this instead of REST.

[andy-k-improving]

@heemin32 done.

[heemin32]

@andy-k-improving, thanks for the change. If you don't get response from @navneet1v by next Monday, let me pull in another person to review this PR.

[navneet1v]

Small comment. Overall code looks good to me. There is system.out statement left in the code. Added a comment for that.

[navneet1v]

The problem with not doing this would be that we first we will not able to test the code with random data and another problem is since we will not have seed whenever we want to reproduce a failure. Not sure why MLCommons didn't do this. This is an acceptable practice in Opensearch Plugins. Plus you will miss out on all the different mocking and testing framework added by Opensearch.