Update GeoGrid base class access modifier to support extensibility

[VijayanB]

Description

• Change access modifier from default to protected. This will help to build new geo based aggregation outside OpenSearch, by keeping GeoGrid Classes as base class.

• Updated GeoTileGrid and GeoHashGrid classes accordingly.

• Updated doc to reflect base class as api

• Renamed API class to reflect that it is not internal anymore

Signed-off-by: Vijayan Balasubramanian balasvij@amazon.com

Issues Resolved

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/3316/
• CommitID: 25573c3

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/3317/
• CommitID: a6f2b5c

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/3319/
• CommitID: 1d949e7

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/3320/
• CommitID: 20075a1

[reta]

@VijayanB all these classes are desribed as internal, we need to update that to reflect that API is public now

* ...
* @opensearch.internal
* ...

See please the relevant discussion here: #3716 (comment), CC @nknize

[reta]

Should we rename these classes to get rid if InternalXxx prefix? It does not look like API for public use, naming should be appropriate

[reta]

Alternatively, we could introduce interfaces (as public APIs) and keep these classes indeed internal

[VijayanB]

We already have public GeoGrid/GeoGridBucket interfaces. I can rename from InternalGeoGrid to AbstractGeoGrid. Do you have any other suggestions?

[reta]

May be GeoGridBase ? The classes are abstract already ....

[navneet1v]

Only in terms of aggregation:
In my mind Internal prefix was added to classes which will be used when data nodes send the response to coordinator for aggregation. Which made sense to me.

Then the other classes GeoGridBucket classes are the actual interfaces which provides the response to customer.

[navneet1v]

Yeah thats a valid point. Thanks for providing this idea. +1 on this.

[VijayanB]

@reta In other classes, i see either BaseInterfaceName or AbstractInterfaceName. i couldn't find any abstract class that ends with "Base". I don't have strong opinion on one over other, do you have? However, i slightly prefer Base as prefix over suffix :)

[reta]

@VijayanB it is up to us :)

[VijayanB]

@reta I updated name with prefix. Can you take another look? Thanks

[reta]

@VijayanB my apologies, missed the notification

[VijayanB]

@VijayanB all these classes are desribed as internal, we need to update that to reflect that API is public now

* ...
* @opensearch.internal
* ...

See please the relevant discussion here: #3716 (comment), CC @nknize
Thanks @reta . I updated the abstract class and interface that will be extended/inherited by plugins outside this repository. Is there anything else pending other than updating doc to convey that these classes are public?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/3341/
• CommitID: d1f1e99

[navneet1v]

I am not sure if we want to add api here. @nknize can get some suggestion from your side.

[navneet1v]

We have changed the modifier for both HashGrid and TileGrid classes. As this of GeoH3 I am not sure we need to make this amount of changes.

[VijayanB]

We have changed the modifier for both HashGrid and TileGrid classes. As this of GeoH3 I am not sure we need to make this amount of changes.

@navneet1v Do we have these changes available in another PR? if not, i believe these changes are required if we want to add new aggregation outside this repository.

[navneet1v]

We have changed the modifier for both HashGrid and TileGrid classes. As this of GeoH3 I am not sure we need to make this amount of changes.

@navneet1v Do we have these changes available in another PR? if not, i believe these changes are required if we want to add new aggregation outside this repository.

What I was saying is in this PR we are making changes in both HashGrid and TileGrid so do we need both?

[VijayanB]

We have changed the modifier for both HashGrid and TileGrid classes. As this of GeoH3 I am not sure we need to make this amount of changes.

@navneet1v Do we have these changes available in another PR? if not, i believe these changes are required if we want to add new aggregation outside this repository.

What I was saying is in this PR we are making changes in both HashGrid and TileGrid so do we need both?

Yes. We are changing access modifier in their base class.

[navneet1v]

We have changed the modifier for both HashGrid and TileGrid classes. As this of GeoH3 I am not sure we need to make this amount of changes.

@navneet1v Do we have these changes available in another PR? if not, i believe these changes are required if we want to add new aggregation outside this repository.

What I was saying is in this PR we are making changes in both HashGrid and TileGrid so do we need both?

Yes. We are changing access modifier in their base class.

Oh my bad. The changes in base class are tricking up to the top level classes.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/3544/
• CommitID: 29f3ef7

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/4059/
• CommitID: c623489

[VijayanB]

precommit failure looks like irrelevant to this PR.

> Task :qa:evil-tests:thirdPartyAudit FAILED
Unnecessary exclusions, following classes have no violations:
   * com.google.common.hash.Striped64$Cell
  * com.google.common.util.concurrent.AbstractFuture$UnsafeAtomicHelper

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/4089/
• CommitID: cd71a7e

[VijayanB]

@reta Can this be merged? Thanks

[reta]

@VijayanB since we already released this functionality in 2.3 , it will be a breaking change if backported to 2.x / 2.3, so backport label was not applied

[VijayanB]

@reta Since this is just refactor ( compile time ) and not limiting existing scope, Will releasing in 2.4 break backward compatibility?

[reta]

The classes / interfaces were renamed, beside just visibility changes, we will just break people's code if someone is building on top of the GeoModulePlugin ...

[VijayanB]

The classes / interfaces were renamed, beside just visibility changes, we will just break people's code if someone is building on top of the GeoModulePlugin ...

Thanks for clarifications. The visibility change is prerequisite for opensearch-project/geospatial#98 . Do you have any recommendation on how to make those visibility changes available to 2.x?

[reta]

The classes / interfaces were renamed, beside just visibility changes, we will just break people's code if someone is building on top of the GeoModulePlugin ...

Thanks for clarifications. The visibility change is prerequisite for opensearch-project/geospatial#98 . Do you have any recommendation on how to make those visibility changes available to 2.x?

I think it will be very difficult to refactor this pull request into something which is backward compatible (with 2.x). We may either make the effort to do that, or break the rule of backward compatibility on the ground this is a new module with high probablility not being used widely (or not backport at all which I belive is not desirable), @andrross @dblock what do you think guys?

[andrross]

@reta For this specific case it looks like we've already backported some breaking changes to 2.x with the rename GeoPlugin->GeoModulePlugin in #4179. Assuming that was released in 2.3 without issue then we can probably continue the pattern here.

[dblock]

Given what @andrross is saying we either have to revert/update #4179, or we can keep YOLOing it and backport this change to 2.x.

To provide a backward compatible interface we could implement InternalGeoHashGrid that extends from the newly refactored GeoHashGrid. The other idea that @VijayanB had in a brainstorm is to backport the change without doing the renaming (don't rename InternalGeoHashGrid to GeoHashGrid in 2.x). I think I really like this last option, WDYT?

[nknize]

The classes / interfaces were renamed, beside just visibility changes, we will just break people's code if someone is building on top of the GeoModulePlugin

GeoModulePlugin does not implement ExtensiblePlugin so it, in and of itself, is not extendable. Technically it should be marked @opensearch.internal w/ a final modifier.

[nknize]

To provide a backward compatible interface we could implement InternalGeoHashGrid that extends from the newly refactored GeoHashGrid.

A couple things here:

1. Technically the Elasticsearch aggregation framework never provided a formal API. This is (partially) why InternalGeoHashGrid was marked @opensearch.internal thus third party users are not guaranteed bwc outside the HLRC DSL.
2. GeoHash is legacy, the boundaries of a geohash grid do not align w/ mapping layers (which are technically web mercator grid). This is why there's now a geo_grid w/ geohash_grid remaining to support analytics w/ geohash encoding only.
3. Moving to a more formalized API in 3.0 w/ the rename from InternalGeoHashGrid to GeoGrid is a good move here that we don't have to guarantee bwc support.. it also allows us to add hex grid as an extension to this same API.
4. As mentioned above, the GeoModulePlugin is a (installed by default) module that does not implement ExtensiblePlugin so users cannot extend this module anyway (something we may reconsider for 3.0).

I'm not sure there's much of a concern here for 2.x given no formal bwc requirement?

Update: Maybe this is the opportunity to signal formal OpenSearch Geo Aggregation APIs effective OpenSearch 3.0?

I'm curious some thoughts from @hanoch's perspective as a geo user / integrator. @hanoch, don't you have some custom geo grid aggregations that extend the internal classes? These were never guaranteed bwc, how do you feel about us supporting a formal geo aggregation api in 3.0? What extension points do you rely on?

[VijayanB]

@nknize I really like the idea that introducing a formal contract for Geo Plugin/Geo features, to enable other plugins to build on top of it with BWC support. Given there is no commitment so far and this PR only refactored Classes that are marked internal, should we back port this PR to 2.x?

[nknize]

should we back port this PR to 2.x?

Yes. Given 3.0 is not far off that would be a good time to introduce a formal contract marked w/ @opensearch.api. Until then, backporting is safe.