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.

Sign up for GitHub

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

Jump to bottom

refactor inner geogrid classes to own class files #37596

Merged
merged 2 commits into from
Jan 18, 2019
Merged

refactor inner geogrid classes to own class files #37596

merged 2 commits into from
Jan 18, 2019

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Jan 17, 2019

To make further refactoring of GeoGrid aggregations
easier (related: #30320), splitting out these inner
class dependencies into their own files makes it
easier to map the relationship between classes

@talevy talevy added :Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring labels Jan 17, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@talevy
refactor inner geogrid classes to own class files
653b518 
To make further refactoring of GeoGrid aggregations
easier (related: elastic#30320), splitting out these inner
class dependencies into their own files makes it
easier to map the relationship between classes
@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 1
run gradle build tests 2

@talevy talevy mentioned this pull request Jan 18, 2019
Closed
nyurik
nyurik approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for getting it started!

@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 2

@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 1

2 similar comments
@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 1

@nyurik
Copy link
Contributor

nyurik commented Jan 18, 2019

run gradle build tests 1

imotov
imotov approved these changes Jan 18, 2019
View reviewed changes
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.

LGTM. I left a couple of minor suggestions. I wonder if it would also make sense to rename everything GeoGrid* into GeoHashGrid* for consistency while you are at it.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java Show resolved Hide resolved
.../java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java Outdated Show resolved Hide resolved
@nyurik
Copy link
Contributor

nyurik commented Jan 18, 2019

... I wonder if it would also make sense to rename everything GeoGrid* into GeoHashGrid* for consistency while you are at it.

@imotov The goal is to actually do the opposite - we are renaming everything into GeoGrid*, plus adding a few tiny classes (extending the base abstract classes) named GeoHashGrid* to handle the specifics of the geohash vs quadkey vs hexgrid vs pluscode vs ...

@imotov
Copy link
Contributor

imotov commented Jan 18, 2019

Makes sense. What about GeoHashGrid and geohashAsLong in GeoGridBucket? Should these become GeoGrid and geogridKey or this will happen in the follow up?

@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

I was planning that type of renaming to be done in a followup, wanted to get these new class files out of the way before further changes to them

@nyurik
Copy link
Contributor

nyurik commented Jan 18, 2019

run the gradle build tests 1

@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

thanks for the reviews @imotov and @nyurik. I'll move forward with the follow-ups after CI is happy

@talevy talevy removed the request for review from polyfractal January 18, 2019 21:15
@talevy talevy added the v7.0.0 label Jan 18, 2019
@talevy talevy merged commit 106f900 into elastic:master Jan 18, 2019
@talevy talevy deleted the refactor-inner-geogrid branch January 18, 2019 21:40
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 21, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into sync-retention-leases
e5bb88c 
* elastic/master: (104 commits)
  Permission for restricted indices (elastic#37577)
  Remove Watcher Account "unsecure" settings (elastic#36736)
  Add cache cleaning task for ML snapshot (elastic#37505)
  Update jdk used by the docker builds (elastic#37621)
  Remove an unused constant in PutMappingRequest.
  Update get users to allow unknown fields (elastic#37593)
  Do not add index event listener if CCR disabled (elastic#37432)
  Add local session timeouts to leader node (elastic#37438)
  Add some deprecation optimizations (elastic#37597)
  refactor inner geogrid classes to own class files (elastic#37596)
  Remove obsolete deprecation checks (elastic#37510)
  ML: Add support for single bucket aggs in Datafeeds (elastic#37544)
  ML: creating ML State write alias and pointing writes there (elastic#37483)
  Deprecate types in the put mapping API. (elastic#37280)
  [ILM] Add unfollow action (elastic#36970)
  Packaging: Update marker used to allow ELASTIC_PASSWORD (elastic#37243)
  Fix setting openldap realm ssl config
  Document the need for JAVA11_HOME (elastic#37589)
  SQL: fix object extraction from sources (elastic#37502)
  Nit in settings.gradle for Eclipse
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyurik nyurik nyurik approved these changes

@imotov imotov imotov approved these changes

Assignees
No one assigned
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@talevy @elasticmachine @nyurik @imotov @colings86