-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ILM] Add support for frozen phase #93068
Conversation
64cdc5a
to
6202e37
Compare
6202e37
to
6fb98fb
Compare
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sebelga! I left some copy suggestions. Let me know if you have any questions.
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts
Outdated
Show resolved
Hide resolved
...ment/public/application/sections/edit_policy/components/phases/frozen_phase/frozen_phase.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts
Outdated
Show resolved
Hide resolved
...ifecycle_management/public/application/sections/edit_policy/components/more_less_section.tsx
Outdated
Show resolved
Hide resolved
<EuiSpacer /> | ||
<FrozenPhase /> | ||
|
||
<EuiSpacer /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a spacer between frozen and delete phases makes the connecting line go beyond frozen phase:
I think this spacer needs to go into delete phase to fix it. There is an issue open in EUI repo to address 'gutter size' between 'comment items' but it is not being worked on currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this, @sebelga!
align?: 'left' | 'right'; | ||
} | ||
|
||
export const MoreLessSection: FunctionComponent<Props> = ({ align = 'left', children }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this component can be replaced with EuiAccordion
, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your reasoning here @sebelga, but then having 2 different animations make the UI inconsistent.
Should we change Advanced settings to the new component too?
I also have concerns about built-in a11y features that handle this kind of interaction in accordion. There are attributes like aria-controls
, aria-expanded
, role
, aria-labelledby
that we need to implement in the new component as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Yulia raises great points about accessibility. My advice would be to leverage EUI as much as possible, and then drive changes at the EUI level to support the behavior you want @sebelga. For example, ask for a timing
or animation
prop to be added to EuiAccordion
. This will have the benefit of getting input from the design team, it will share the new behavior to all EUI consumers to reduce the potential for duplication, and it will ensure consistency throughout our UIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big +1 to using EUI components and requesting desired enhancements on their repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An additional future consideration here... there is an existing Advanced setting to 'Disable animations' globally. Imagine a future - with user settings - where an individual can toggle this as their personal preference.
I only recently learned of this setting and just now tried it out - as a quick test, it removed the animation from the accordions in the Canvas sidebar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about accessibility @yuliacech and leverage EUI effort on this, I will replace the component with the accordion 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue with the <EuiAccordion />
that clips the combox helptext after the first line. I've opened an issue for it (elastic/eui#4623). Should we block this PR until the issue is solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sebelga, thanks a lot for working on this!
Left 2 comments in the code, curios what you think.
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
...ment/public/application/sections/edit_policy/components/phases/frozen_phase/frozen_phase.tsx
Outdated
Show resolved
Hide resolved
...ts/phases/shared_fields/data_tier_allocation_field/components/missing_cloud_tier_callout.tsx
Show resolved
Hide resolved
...cy/components/phases/shared_fields/data_tier_allocation_field/data_tier_allocation_field.tsx
Outdated
Show resolved
Hide resolved
...licy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sebelga,
thanks a lot for working on this! I re-tested locally and all worked for me. Left a couple of comments in the code, but nothing blocking.
Thanks for the review @yuliacech ! I addressed your last requests for change 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reaching out offline @sebelga. I've included some updated copy for the searchable snapshots field descriptions.
@dakrone Let me know if you have any feedback on this copy. Thanks!
Searchable snapshot for hot and cold tiers
Take a snapshot of your data and mount it as a searchable snapshot. <Learn more>.
The <Learn more>
link remains the same.
Searchable snapshot for frozen tiers
Take a snapshot of your data and mount it as a searchable snapshot. To reduce costs, only a cache of the snapshot is mounted in the frozen tier. <Learn more>.
<Learn more>
should link to https://www.elastic.co/guide/en/elasticsearch/reference/master/searchable-snapshots.html#searchable-snapshots-shared-cache
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts
Outdated
Show resolved
Hide resolved
…ections/edit_policy/i18n_texts.ts Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
…lm-ui/frozen-tier
Thanks for the quick update @jrodewig ! Your suggestions look good to me. note: hot phase has the same description for searchable snapshot as cold phase |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…-action * 'master' of github.com:elastic/kibana: (43 commits) [Console] Update copy when showing warnings in response headers (#94270) [TSVB] Type public code. Step 1 (#93231) [ML] Functional tests - stabilize slider value selection (#94313) skip another suite blocking es promotion (#94367) [Security Solution] Eliminates a redundant external link icon (#94194) skip another suite blocking es promotion (#94367) [App Search] Role mappings migration part 1 (#94346) [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241) [Workplace Search] Deduplicate icons (#94359) [ML] Add latest transform to intro text (#94039) skip test failing es promotion (#94367) [Maps] convert elasticsearch_utils to TS (#93984) [Security_Solution][Telemetry] - Update endpoint usage to use agentService (#93829) [Security Solution][Exceptions] Fixes OS adding method for exception enrichment (#94343) [ILM] Add support for frozen phase (#93068) [App Search] Fixed 2 relevance tuning bugs (#94312) remove `try` auth mode (#94287) Removing resolver functional tests (#94331) migrate warning mixin to core (#94273) [App Search] Add routes for Role Mappings (#94221) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_issues_context.tsx
This PR adds support for the
frozen
phase and thefrozen
tier in ILM. The frozen phase is following the same rules as the cold phase with the exception that the frozen phase defaultssearchable_snapshot.storage
toshared_cache
where incold
phase it isfull_copy
.License
Frozen phase has only one action: searchable snapshot. As searchable snapshot requires the entreprise license, the frozen phase is only visible for users with that license. In
basic
license the frozen phase is not shown.How to test
storage
optionTo simulate that Kibana runs in cloud context, add the following in the
kibana.dev.yml
Release note
You can now configure the
frozen
phase in your index lifecycle policy. This new data tier is optimized for maximum cost savings.