Skip to content

refactor inner geogrid classes to own class files#37596

Merged
talevy merged 2 commits intoelastic:masterfrom
talevy:refactor-inner-geogrid
Jan 18, 2019
Merged

refactor inner geogrid classes to own class files#37596
talevy merged 2 commits intoelastic:masterfrom
talevy:refactor-inner-geogrid

Conversation

@talevy
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Jan 17, 2019

Pinging @elastic/es-analytics-geo

@talevy talevy requested review from imotov and polyfractal January 17, 2019 23:26
@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 talevy force-pushed the refactor-inner-geogrid branch from 365c80e to 653b518 Compare January 17, 2019 23:51
@talevy
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 2

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 1

2 similar comments
@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 18, 2019

run gradle build tests 1

@nyurik
Copy link
Copy Markdown
Contributor

nyurik commented Jan 18, 2019

run gradle build tests 1

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

@nyurik
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

nyurik commented Jan 18, 2019

run the gradle build tests 1

@talevy
Copy link
Copy Markdown
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

2 more reviewers

@imotov imotov imotov approved these changes

@nyurik nyurik nyurik approved these changes

Reviewers whose approvals may not affect merge requirements

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