Move legacy geo_shape implementation to its own module #77856
Conversation
iverase
added
:Analytics/Geo
elasticmachine
added
Team:Analytics
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
x-pack/plugin/spatial/build.gradle
Outdated
| @@ -11,6 +11,7 @@ esplugin { | |||
| } | |||
|
|
|||
| dependencies { | |||
| api project(path: ':modules:legacy-geo') | |||
There was a problem hiding this comment.
Just to be clear, this will cause a copy of the legacy-geo jar (and it's dependencies) to be added to the xpack spatial module. It won't reuse the same. I think that is ok, as there is no communication between them, I believe this is just for reuse of some common stuff, but I want to double check with you of that understanding.
There was a problem hiding this comment.
We need this because when creating a geo_shape field mapper we might need to create the mapper on the legacy jar:
There was a problem hiding this comment.
We need this because when creating a geo_shape field mapper we might need to create the mapper on the legacy jar:
There was a problem hiding this comment.
Is that the only use? If so, consider moving the shared code into a project under lib. By depending on the module the dependencies, including jts, are pulled into (as a copy) the spatial module.
There was a problem hiding this comment.
I think it is tricky, the mapper and shape builders companions are tightly dependent on Elasticsearch server and other projects under lib.
There was a problem hiding this comment.
No, I don’t think that is an issue. The mapper is checking the existing field type against itself.
There was a problem hiding this comment.
I think there is no interaction between the modules except for the spatial module creating Legacy mappers under certain circumstances. Still I am thinking if we can do better without being too invasive.
There was a problem hiding this comment.
How does spatial create the legacy mappers, through normal mapper apis (ie json) or directly in code? If it is in code, then I don't think that would work.
The "solution" is to do something we recommend against in general, which is declaring the spatial module "extends" legacy geo. Even though it doesn't actually extend it, it will make the classloader of legacy geo a parent of spatial, so the classes would be available. To do this you would add the new module to the extendedPlugins list in build.gradle, and change the dependencies section to use compileOnly instead of api for the legacy module.
There was a problem hiding this comment.
I implemented the "solution" above, therefore now spatial module extends legacy module and dependency
is compile only now. It was very insightful and I got a better understanding how it works. This action has some side effects:
- Vector tile extends the spatial module but now in order to compile I need to add a compile only dependency to JTS
- Couple of testing modules were adding spatial as a runtime dependency. I have to add legacy-geo as well to make it happy.
There was a problem hiding this comment.
@rjernst Do you think this is good enough or we should have more iterations?
# Conflicts: # modules/legacy-geo/licenses/lucene-spatial-extras-8.10.0-snapshot-bf2fcb53079.jar.sha1
# Conflicts: # modules/legacy-geo/licenses/lucene-spatial-extras-9.0.0-snapshot-32a0a16aff0.jar.sha1 # x-pack/plugin/sql/qa/server/build.gradle
|
@elasticmachine update branch |
|
@elasticmachine update branch |
This commit moves the legacy geo_shape implementation to its own module and removes the dependency on Spatial4J / JTS from server.
This PR moves the legacy geo_shape implementation to its own module and removes the dependency on Spatial4J / JTS from server. In addition moves the dependency with lucine-spatial-extras away from server.
Unfortunately the diff is pretty big due to run spotless in the new module, but the PR is mostly moving files around.