Clear out old rollup strategy by lizozom · Pull Request #59423 · elastic/kibana

lizozom · 2020-03-05T14:12:27Z

Summary

Depends on #59224

Don't reguster Rollup strategy into the data strategy registry
Always use defaultSearchStrategy in callClient
Pass down the indexType to the server side
Return an error if an OSS setup tries to search a rollup index pattern
Clear out some types

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

lizozom · 2020-03-05T17:57:19Z

@elasticmachine merge upstream

elasticmachine · 2020-03-05T17:57:21Z

merge conflict between base and head

…earch/clear-out-old-rollup-strategy

elasticmachine · 2020-03-08T13:22:18Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

…earch/clear-out-old-rollup-strategy

alvarezmelissa87

ML bit LGTM 👍

lukasolson

Tested and things seem to be functioning well.

A couple of things we should probably note: The original rollup search strategy only sent the aggs/query/index/size from searchRequest, not everything else (like stored_fields, script_fields, docvalue_fields). We should probably do something similar:

kibana/x-pack/legacy/plugins/rollup/public/search/rollup_search_strategy.ts

Line 23 in 9985597

body: { size, aggs, query: _query },

Other than that, left a couple of comments below.

src/plugins/data/public/index_patterns/lib/get_index_pattern_type.ts

src/plugins/data/public/search/fetch/call_client.test.ts

lukasolson · 2020-03-10T22:58:18Z

src/plugins/data/public/index.ts

  SavedQuery,
  SavedQueryService,
  SavedQueryTimeFilter,
-  SavedQueryAttributes,


I'm not sure why changes to SavedQueryAttributes and TimefilterSetup are included in this PR... Could you explain?

Cleaning up while working.
Not related, but didn't feel like dropping those either.

…earch/clear-out-old-rollup-strategy

kertal

Nice that rollups are supported now, but there's one thing to discuss, concerning Discover. the rollup @timestamp works in a different way, it's not a single value. Therefore it would need adaptions, because currently the "Time" column is empty when a rollup pattern is used. also sorting and time filter don't work

So I think it's better to keep the warning that the index pattern is not supported, and to open an issue "Support rollup index patterns" in discover

lizozom · 2020-03-11T08:47:54Z

@lukasolson I addressed most of your feedback.
However, as FE is now unaware of whether a rollup \ regular strategy is about to be used, why not let the server pick the arguments it wants?

kertal

Code LGTM, wasn't aware that the handling of rollup data in Discover is no regression, but a thing that should be improved in the future.

lizozom · 2020-03-11T12:27:08Z

@elasticmachine merge upstream

…rategy

…earch/clear-out-old-rollup-strategy

…thub.com:lizozom/kibana into newplatform/search/clear-out-old-rollup-strategy

kibanamachine · 2020-03-11T15:41:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 11914e1

History

💔 Build #32382 failed 6a461d7
💚 Build #32114 succeeded f80efb0
💚 Build #32056 succeeded 73eb852
💔 Build #32025 failed 3a5fcc2
💔 Build #31998 failed 241c247

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lukasolson

LGTM!

* Old search strategy cleanup * restore rollup strategy * Remove hasSearchStategyForIndexPattern * ts * fix jest tests * cleanup exports * pass index pattern type to server for rollups * merge fix * Fix types * ts fixes * oss strategy error handling * update translations * jest test fix * Use indexType instead of index * code review 1 * updated docs * jest test * jest snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Old search strategy cleanup * restore rollup strategy * Remove hasSearchStategyForIndexPattern * ts * fix jest tests * cleanup exports * pass index pattern type to server for rollups * merge fix * Fix types * ts fixes * oss strategy error handling * update translations * jest test fix * Use indexType instead of index * code review 1 * updated docs * jest test * jest snapshot Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

lizozom self-assigned this Mar 5, 2020

Old search strategy cleanup

820a256

lizozom force-pushed the newplatform/search/clear-out-old-rollup-strategy branch from 19b0f52 to 820a256 Compare March 6, 2020 18:09

Liza K added 3 commits March 8, 2020 10:52

Merge branch 'master' of github.com:elastic/kibana into newplatform/s…

8f70261

…earch/clear-out-old-rollup-strategy

restore rollup strategy

01bc8ce

Remove hasSearchStategyForIndexPattern

9ee289d

lizozom marked this pull request as ready for review March 8, 2020 13:16

lizozom requested a review from a team March 8, 2020 13:16

lizozom requested a review from a team as a code owner March 8, 2020 13:16

lizozom requested review from kertal and lukasolson March 8, 2020 13:16

ts

a336c5a

lizozom added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.7.0 v8.0.0 labels Mar 8, 2020

Liza K added 3 commits March 8, 2020 23:16

fix jest tests

db427e7

Merge branch 'master' of github.com:elastic/kibana into newplatform/s…

162abe9

…earch/clear-out-old-rollup-strategy

cleanup exports

417623b

lizozom requested a review from a team as a code owner March 9, 2020 12:28

Liza K added 6 commits March 9, 2020 19:45

pass index pattern type to server for rollups

1c6b42c

Merge branch 'master' of github.com:elastic/kibana into newplatform/s…

004f0f4

…earch/clear-out-old-rollup-strategy

merge fix

6099189

Fix types

02a101a

ts fixes

d5f9f8a

oss strategy error handling

241c247

Liza K added 3 commits March 10, 2020 13:20

update translations

3a5fcc2

jest test fix

73eb852

Use indexType instead of index

f80efb0

alvarezmelissa87 approved these changes Mar 10, 2020

View reviewed changes

lukasolson reviewed Mar 10, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into newplatform/s…

4fc6ed9

…earch/clear-out-old-rollup-strategy

kertal reviewed Mar 11, 2020

View reviewed changes

Liza K added 2 commits March 11, 2020 10:56

code review 1

8c36948

updated docs

90f05ec

kertal mentioned this pull request Mar 11, 2020

[Discover] Reimplement $route.reload when index pattern changes #59877

Merged

kertal self-requested a review March 11, 2020 10:09

kertal approved these changes Mar 11, 2020

View reviewed changes

jest test

73b73d8

elasticmachine and others added 4 commits March 11, 2020 08:27

Merge branch 'master' into newplatform/search/clear-out-old-rollup-st…

6a461d7

…rategy

Merge branch 'master' of github.com:elastic/kibana into newplatform/s…

03ad503

…earch/clear-out-old-rollup-strategy

jest snapshot

ca43314

Merge branch 'newplatform/search/clear-out-old-rollup-strategy' of gi…

11914e1

…thub.com:lizozom/kibana into newplatform/search/clear-out-old-rollup-strategy

lukasolson approved these changes Mar 11, 2020

View reviewed changes

lizozom merged commit 6645531 into elastic:master Mar 11, 2020

lizozom mentioned this pull request Mar 11, 2020

[7.x] Clear out old rollup strategy (#59423) #59921

Merged

lukeelmers mentioned this pull request Mar 11, 2020

Cutover aggs to the new platform #59605

Merged

lukasolson added the Feature:Search Querying infrastructure in Kibana label Mar 26, 2020

lukasolson mentioned this pull request Mar 26, 2020

[Search service] Remove old search strategies #54821

Closed

Conversation

lizozom commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

lizozom commented Mar 5, 2020

Uh oh!

elasticmachine commented Mar 5, 2020

Uh oh!

elasticmachine commented Mar 8, 2020

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukasolson Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

kertal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom commented Mar 11, 2020

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom commented Mar 11, 2020

Uh oh!

kibanamachine commented Mar 11, 2020

💚 Build Succeeded

History

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

lizozom commented Mar 5, 2020 •

edited

Loading

kertal left a comment •

edited

Loading