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

Ensure that ip_range aggregations always return bucket keys. #30701

Merged
merged 2 commits into from
May 24, 2018
Merged

Ensure that ip_range aggregations always return bucket keys. #30701

merged 2 commits into from
May 24, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented May 17, 2018 •
edited
Loading

Addresses #21045.

@colings86 colings86 added the :Analytics/Aggregations Aggregations label May 18, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani added v7.0.0 and removed WIP labels May 18, 2018
@jtibshirani jtibshirani force-pushed the bugfix/ip-range-keys branch 3 times, most recently from fe175fd to e37f68f Compare May 21, 2018 19:29
@jtibshirani jtibshirani requested a review from jpountz May 22, 2018 03:09
jpountz
jpountz reviewed May 22, 2018
View reviewed changes
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 left a comment about serialization but it looks good to me in general.

...er/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java Outdated
}

private static Bucket createFromStream(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
String key = in.readOptionalString();
Copy link
Contributor

Choose a reason for hiding this comment

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

given that the key is never null anymore, let's write it with writeString and read it with readString?

Copy link
Contributor Author

@jtibshirani jtibshirani May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I had opted not to change this because of the many follow-up changes required. I'm happy to make the change though, just to check I have all the steps right:

  • Change this to readString if the input stream has version >= 7.0.0 (and make the corresponding change for writing). Merge the PR.
  • Backport to 6.x, changing the version check to >= 6.4.0.
  • On master, also change the version check to >= 6.4.0.
  • Likely do the same fix for InternalRange (it also uses writeOptionalString).

Copy link
Contributor

Choose a reason for hiding this comment

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

You got the steps right. I agree this is a bit painful but I think this is worth doing otherwise we are accumulating tech debt and someone who doesn't know about the history of this class will wonder why we have an optional two years from now. For the record, I assumed this PR was 7.0-only based on the label, which would have made the serialization change easier to merge.

+1 to fix InternalRange as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I made the change to readString. And makes sense about the missing PR label!

@jtibshirani jtibshirani merged commit 638a719 into elastic:master May 24, 2018
@jtibshirani jtibshirani deleted the bugfix/ip-range-keys branch May 24, 2018 15:55
jtibshirani added a commit that referenced this pull request May 24, 2018
@jtibshirani
Ensure that ip_range aggregations always return bucket keys. (#30701)
49f1cec 
(cherry picked from commit 638a719)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 24, 2018
@jasontedor
Merge branch 'master' into verify-official-plugin-signature
9d40715 
* master:
  Update the version checks around ip_range bucket keys, now that the change was backported.
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Use geohash cell instead of just a corner in geo_bounding_box (elastic#30698)
  Limit user to single concurrent auth per realm (elastic#30794)
  [Tests] Move templated _rank_eval tests (elastic#30679)
  Security: fix dynamic mapping updates with aliases (elastic#30787)
  Ensure that ip_range aggregations always return bucket keys. (elastic#30701)
  Use remote client in TransportFieldCapsAction (elastic#30838)
  Move Watcher versioning setting to meta field (elastic#30832)
  [Docs] Explain incomplete dates in range queries (elastic#30689)
  Move persistent task registrations to core (elastic#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (elastic#30809)
  Send client headers from TransportClient (elastic#30803)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (elastic#30732)
  Force stable file modes for built packages (elastic#30823)
  [DOCS] Fixes typos in security settings
  Fix GeoShapeQueryBuilder serialization after backport
martijnvg added a commit that referenced this pull request May 25, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
e6dd414 
* es/master:
  Move score script context from SearchScript to its own class (#30816)
  Fix bad version check writing Repository nodes (#30846)
  [docs] explainer for java packaging tests (#30825)
  Remove Throwable usage from transport modules (#30845)
  REST high-level client: add put ingest pipeline API (#30793)
  Update the version checks around ip_range bucket keys, now that the change was backported.
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Use geohash cell instead of just a corner in geo_bounding_box (#30698)
  Limit user to single concurrent auth per realm (#30794)
  [Tests] Move templated _rank_eval tests (#30679)
  Security: fix dynamic mapping updates with aliases (#30787)
  Ensure that ip_range aggregations always return bucket keys. (#30701)
  Use remote client in TransportFieldCapsAction (#30838)
  Move Watcher versioning setting to meta field (#30832)
  [Docs] Explain incomplete dates in range queries (#30689)
  Move persistent task registrations to core (#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (#30809)
  Send client headers from TransportClient (#30803)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732)
  Force stable file modes for built packages (#30823)
martijnvg added a commit that referenced this pull request May 25, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
545ed87 
* es/6.x:
  Move score script context from SearchScript to its own class (#30816)
  Fix bad version check writing Repository nodes (#30846)
  Modify state of VerifyRepositoryResponse for bwc (#30762)
  QA: Fix tribe tests when running default zip
  Use remote client in TransportFieldCapsAction (#30838)
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Ensure that ip_range aggregations always return bucket keys. (#30701)
  Limit user to single concurrent auth per realm (#30794)
  Security: fix dynamic mapping updates with aliases (#30787)
  [Tests] Move templated _rank_eval tests (#30679)
  Move Watcher versioning setting to meta field (#30832)
  Restore "Add more yaml tests for get alias API " (#30814)
  Send client headers from TransportClient (#30803)
  [Docs] Explain incomplete dates in range queries (#30689)
  Move persistent task registrations to core (#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (#30809)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732)
  Force stable file modes for built packages (#30823)
@jtibshirani
Copy link
Contributor Author

I've also updated InternalRange to serialize non-optional strings here: #30886

@Mpdreamz Mpdreamz mentioned this pull request Sep 25, 2018
Closed
89 tasks
Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Oct 1, 2018
@Mpdreamz
make sure we parse the bucket key on ip range as expected now that it…
fa8c5c8 
…s always returned as per elastic/elasticsearch#30701
@Mpdreamz Mpdreamz mentioned this pull request Oct 1, 2018
Merged
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Oct 17, 2018
@Mpdreamz @russcam
Parse buckets key on ip range aggregation (#3446)
13867d0 
as per elastic/elasticsearch#30701
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Oct 26, 2018
@Mpdreamz @russcam
Parse buckets key on ip range aggregation (#3446)
4cc8fa2 
as per elastic/elasticsearch#30701 

(cherry picked from commit 13867d0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >bug v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtibshirani @elasticmachine @jpountz @colings86