Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GEO] Fork Lucene's LatLonShape Classes to local lucene package #36794

Merged
merged 4 commits into from Dec 18, 2018

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Dec 18, 2018

Lucene 7.6 uses a smaller encoding for LatLonShape. This commit forks the LatLonShape classes to Elasticsearch's local lucene package. These classes will be removed on the release of Lucene 7.6.

Lucene 7.6 uses a smaller encoding for LatLonShape. This commit forks the LatLonShape classes to Elasticsearch's local lucene package. These classes will be removed on the release of Lucene 7.6.
@nknize nknize added >feature :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 labels Dec 18, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@nknize
Copy link
Contributor Author

nknize commented Dec 18, 2018

@imotov having some issues w/ sql-cli and LatLonShape forbiddenAPI signatures. Would you mind having a look?

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be more a build issue than a sql-cli issue. It looks like the forbidden APIs in es-server-signatures.txt are applied to all modules rather than just the server module like the name suggests, which turns out to be an issue for modules that don't depend on lucene-sandbox where LatLonShape lives. Maybe the best way to proceed in the short term is to have a dedicated signatures file for these classes that live in sandbox and add them only in the Gradle build file for the server module (see eg. how rest-high-level-client specific signatures are added in client/rest-high-level/build.gradle)? @atorok Any opinion?

@imotov
Copy link
Contributor

imotov commented Dec 18, 2018

@nknize found it. Running the rest of precommit to make sure it didn't break anything else and then will push the changes.

SQL cli doesn't use the server and doesn't pull most of the server's
dependencies. So, we shouldn't try checking server's signatures here.
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with Igor's workaround.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM2

@nknize
Copy link
Contributor Author

nknize commented Dec 18, 2018

Excellent... as soon as CI comes back (hopefully succesfully) I will merge this to master. We'll let CI settle out overnight with these two big PRs and if all looks good tomorrow (and approvals are in), I'll merge #36743 and backport this to 6.6

@nknize nknize added the v6.6.0 label Dec 18, 2018
@nknize nknize merged commit 20b58f0 into elastic:master Dec 18, 2018
nknize added a commit to nknize/elasticsearch that referenced this pull request Dec 18, 2018
…tic#36794)

Lucene 7.6 uses a smaller encoding for LatLonShape. This commit forks the LatLonShape classes to Elasticsearch's local lucene package. These classes will be removed on the release of Lucene 7.6.
@alpar-t
Copy link
Contributor

alpar-t commented Dec 19, 2018

@jpountz The reason we configure the server signatures is that it's possible for projects depending on server to use the APIs forbidden in server via transitive dependencies. We could however be smarter about it and only add those signature if the project actually depends on server. I'll send a PR to make this happen.

@iverase
Copy link
Contributor

iverase commented Dec 19, 2018

I run the command ./gradlew assemble and got the following error:

> Task :server:javadoc
/Users/ivera/forks/elasticsearch-fork/server/src/main/java/org/apache/lucene/geo/XRectangle2D.java:41: error: unknown tag: lucene.internal
 * @lucene.internal
   ^
/Users/ivera/forks/elasticsearch-fork/server/src/main/java/org/apache/lucene/geo/XTessellator.java:67: error: unknown tag: lucene.experimental
 * @lucene.experimental
   ^
/Users/ivera/forks/elasticsearch-fork/server/src/main/java/org/apache/lucene/document/XLatLonShape.java:55: error: unknown tag: lucene.experimental
 * @lucene.experimental
   ^
3 errors

> Task :server:javadoc FAILED

Removing the Lucene tags remove the problem.

iverase added a commit to iverase/elasticsearch that referenced this pull request Dec 19, 2018
Remove lucene tags as they break gradle javadoc job

Relates elastic#36794
iverase added a commit that referenced this pull request Dec 19, 2018
Remove lucene tags as they break gradle javadoc task

Relates #36794
nknize pushed a commit to nknize/elasticsearch that referenced this pull request Dec 19, 2018
Remove lucene tags as they break gradle javadoc task

Relates elastic#36794
nknize pushed a commit that referenced this pull request Dec 19, 2018
Remove lucene tags as they break gradle javadoc task

Relates #36794
nknize pushed a commit to nknize/elasticsearch that referenced this pull request Dec 19, 2018
Remove lucene tags as they break gradle javadoc task

Relates elastic#36794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >feature v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants