[ILM] Support max primary shard size rollover field #96545

jloleysens · 2021-04-08T10:31:00Z

Summary

Fixes #92037

Screenshots

How to test

Start Kibana and ES yarn es snapshot; yarn start
Go to the Index Lifeccycle Management subsection of the Management section
Create or edit a policy
Expand the Hot Phase advanced settings
Ensure "use default rollover" is not selected
Add a value to the "Maximum index size" field
Check the deprecation message is showing
Check that the "Maximum primary shard size" field is being added to the policy if there is a value, check that it is being removed if there is no value

For copy review

L26 of x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/components/max_index_size_field.tsx
L111 x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx

Release note

The ILM policy configuration UI now supports the new maximum primary index size field in the rollover action.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

…elds to their own components

- fixed behaviour to show missing value for rollover - updated jest snapshots

jloleysens · 2021-04-12T14:14:35Z

@elasticmachine merge upstream

elasticmachine · 2021-04-12T14:14:50Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

sebelga

Great work @jloleysens ! I tested locally and works as expected.

sebelga · 2021-04-13T15:10:39Z

...management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx

-                  </EuiFlexItem>
-                </EuiFlexGroup>
+
+                <MaxPrimaryShardSizeField />


Thanks for splitting it up in separate components!

sebelga · 2021-04-13T15:12:13Z

...n/sections/edit_policy/components/phases/hot_phase/use_rollover_value_required_validation.ts

+        (validation) => validation.code === ROLLOVER_VALUE_REQUIRED_VALIDATION_CODE
+      )
+    );
+  }, [form, formData]);


nit: to be slightly more precise and avoid triggering the effect whenever the form state changes you could just provide the getFields handler in the dependency and not the complete form object.

sebelga · 2021-04-13T15:20:56Z

...ugins/index_lifecycle_management/public/application/sections/edit_policy/form/validations.ts

  }
 };

-export const integerValidator: ValidationFunc<FormInternal, string, string> = (arg) => {
+export const integerValidator: ValidationFunc<any, string, string> = (arg) => {


I see why you've added the any here. I solved this kind of problem by not typing the schema

// schema.ts (L136) export const getSchema = (isCloudEnabled: boolean): FormSchema => ({

So I could correctly type here the validation.

…ax_primary_shard_size * 'master' of github.com:elastic/kibana: (99 commits) added missing optional chain for bracket notation (elastic#96939) [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748) [TSVB] Fix per-request caching of index patterns (elastic#97043) [Datatable] Fix filter cell flakiness (elastic#96934) Unskip heatmap suite and fixes flakiness (elastic#96941) [Fleet] Improve performance of data stream API (elastic#97058) [ML] Data Frame Analytics: remove beta badge (elastic#96977) [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251) Instances latency distribution chart tooltips and axis fixes (elastic#95577) [Monitoring] Using primary average shard size (elastic#96177) [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028) ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676) Index pattern field editor - Add warning on name or type change (elastic#95528) [App Search] Add small engine breadcrumb utility helper (elastic#96917) Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012) [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011) Index patterns server - throw correct error on field caps 404 (elastic#95879) Use `EuiThemeProvider` in lists plugin tests and stories (elastic#96129) [npm] upgrade caniuse database (elastic#97002) chore(NA): moving @kbn/apm-utils into bazel (elastic#96227) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/serialization/policy_serialization.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts

ryankeairns · 2021-04-14T13:12:22Z

...ion/sections/edit_policy/components/field_deprecation_warning/field_deprecation_warning.scss

@@ -0,0 +1,13 @@
+.ilmFieldDeprecationWarning {


I'll take a look locally, but these negative margins have me wondering how well this will hold up over time/cross-browser/responsively. Perhaps we could avoid floating the icon to the left of the input by using a prepend or append? https://elastic.github.io/eui/#/forms/form-controls#prepend-and-append

After checking this out a bit more, I do think we should take a different approach here. At smaller sizes (granted its not the most common use case) the warning is not visible. Further, since this triggers for any value, then we should just show it by default / all the time. The prepend or append approach achieves this result.

Hey @ryankeairns ! Thanks for the review and checking out the code locally, happy to make adjustments per your recommendations.

Further, since this triggers for any value, then we should just show it by default / all the time

I was considering this in the initial implementation, but I was concerned that always having the yellow alert logo visible would draw the users attention where it may not be needed. I'd be curious to get your thoughts on this:

We could also show the prepend when the user enters a value since I don't think the pop in effect is particularly disorientating in this case:

CC @mdefazio

Thanks for trying this out. I'll defer to DeFazio on the final adjustments.

ryankeairns

See comments below about handling the warning/icon in a different way. Thanks

ryankeairns · 2021-04-14T13:34:19Z

...ion/sections/edit_policy/components/field_deprecation_warning/field_deprecation_warning.scss

@@ -0,0 +1,13 @@
+.ilmFieldDeprecationWarning {


After checking this out a bit more, I do think we should take a different approach here. At smaller sizes (granted its not the most common use case) the warning is not visible. Further, since this triggers for any value, then we should just show it by default / all the time. The prepend or append approach achieves this result.

cjcenizal · 2021-04-14T14:45:03Z

@jloleysens @mdefazio @ryankeairns Would this use case be aided by elastic/eui#1200, which requests support for warning and success styles for form row error messages?

debadair

Just a couple minor changes. Otherwise LGTM.

debadair · 2021-04-14T21:12:31Z

...ication/sections/edit_policy/components/phases/hot_phase/components/max_index_size_field.tsx

+    'xpack.indexLifecycleMgmt.hotPhase.maximumIndexSizeDeprecationMessage',
+    {
+      defaultMessage:
+        'Maximum index size is deprecated and will be removed in future versions of the Elastic stack. Please use maximum primary shard size instead.',


Suggested change

'Maximum index size is deprecated and will be removed in future versions of the Elastic stack. Please use maximum primary shard size instead.',

'Maximum index size is deprecated and will be removed in a future version. Use maximum primary shard size instead.',

debadair · 2021-04-14T21:18:33Z

...management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx

@@ -107,7 +108,7 @@ export const HotPhase: FunctionComponent = () => {
                  <EuiSpacer size="s" />
                  <FormattedMessage
                    id="xpack.indexLifecycleMgmt.editPolicy.hotPhase.rolloverDefaultsTipContent"
-                    defaultMessage="Rollover when an index is 30 days old or reaches 50 gigabytes."
+                    defaultMessage="Rollover when an index is 30 days old or the primary shard reaches 50 gigabytes."


An index can have multiple primary shards.

Suggested change

defaultMessage="Rollover when an index is 30 days old or the primary shard reaches 50 gigabytes."

defaultMessage="Roll over when an index is 30 days old or any primary shard reaches 50 gigabytes."

mdefazio · 2021-04-14T21:28:59Z

Apologies for the slow response on this. We took this opportunity to go through this as an example PR with @dborodyansky .

@cjcenizal I think that approach could definitely work. We'll have to see if there are any spacing / length issues because these are 2 form fields next to each, so would the help text from one form field be able to extend under the next one (maybe i'm missing an easy implementation here, but this sounds difficult). So we'd likely have a line break in this text and it take up a lot of space. Maybe that's ok though.

If we go with the prepend approach, a few thoughts:

I'm inclined to say that keeping the prepend there always isn't a big deal. But I suppose it depends how obvious we want it to be when they fill it in.
I don't think the yellow warning icon would pass a contrast test on the gray background. So I'd suggest just using the text color. If we decide to only show it on change in the value then this will still be obvious enough IMO.

I'm showing the example if we keep the icon persistent

A few other things that may or may not be relevant for this PR, but wanted to note:

I think the gutter width on the form rows is a bit far and so the 'gigabytes' feels like a different field as opposed to related to the number input.
There's some slight misalignment with the form fields that I think just adding alignItems: 'flexEnd might fix (since one field has a label and the other doesn't).

cjcenizal · 2021-04-14T21:46:48Z

Another option would be to just change the label to Maximum index size (deprecated). We could even add a tooltip to the label to guide people to use Maximum primary shard size instead. Has the benefit of not needing any new design patterns or states.

- implmented always showing the warning icon and updated the color - cleaned up test helpers

by reducing the gutter size before defazios feedback - fixed alignment issue between numeric rollover inputs and their unit select field by adding flex-end styling to the groups

jloleysens · 2021-04-15T09:00:46Z

@mdefazio @ryankeairns @debadair @cjcenizal

Thank you all for the great feedback! I believe this PR is ready for another round of review.

After some thought, I landed on the following approach:

Always show the alert icon (not just when a value is present)
Use a higher contrast, default color

This approach reduces code changes (we don't have additional states to track) so if we ever want to change this it will be really simple. Additionally, I opted for the prepended icon tip instead of adding (deprecated) (?) to the field label as this looks more polished.

(Screenshots with flex align and gutter sizing adjustments)

@sebelga not sure whether you want to go over the code again, I'm quite confident we have not introduced regressions since your last review since conditional logic has been removed.

sebelga · 2021-04-15T10:08:19Z

Reviewed your commits from yesterday and today and LGTM! 👍

kibanamachine · 2021-04-15T11:00:02Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`indexLifecycleManagement`	200	206	+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexLifecycleManagement`	241.9KB	246.2KB	+4.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`indexLifecycleManagement`	50.1KB	50.1KB	+14.0B

History

💚 Build #119551 succeeded 7d4e546
💚 Build #118732 succeeded 5a9f9b3
💔 Build #118684 failed 6d2977c
💚 Build #118238 succeeded faca1e7
💔 Build #118166 failed fdd4f1f

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

mdefazio

LGTM!

jloleysens · 2021-04-15T16:01:58Z

I am going to go ahead and merge this PR since I think we have addressed all outstanding questions regarding the deprecated field (including requested copy changes).

* added max primary shard size rollover field * large-ish refactor of the hot phase component to move the rollover fields to their own components * added comment * address i18n issue * - fixed jest tests - fixed behaviour to show missing value for rollover - updated jest snapshots * added field deprecation component * added test for whether deprecation icon is visible * remove unused import, remove type generic from FormSchema * fixed getting "getFields" from incorrect object * wip!!! * - removed FieldDeprecationWarning component and associated tests - implmented always showing the warning icon and updated the color - cleaned up test helpers * - more tightly grouped the numeric inputs with their unit selects by reducing the gutter size before defazios feedback - fixed alignment issue between numeric rollover inputs and their unit select field by adding flex-end styling to the groups Co-authored-by: Kibana Machine <[email protected]>

* added max primary shard size rollover field * large-ish refactor of the hot phase component to move the rollover fields to their own components * added comment * address i18n issue * - fixed jest tests - fixed behaviour to show missing value for rollover - updated jest snapshots * added field deprecation component * added test for whether deprecation icon is visible * remove unused import, remove type generic from FormSchema * fixed getting "getFields" from incorrect object * wip!!! * - removed FieldDeprecationWarning component and associated tests - implmented always showing the warning icon and updated the color - cleaned up test helpers * - more tightly grouped the numeric inputs with their unit selects by reducing the gutter size before defazios feedback - fixed alignment issue between numeric rollover inputs and their unit select field by adding flex-end styling to the groups Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

jloleysens added 3 commits April 8, 2021 11:49

added max primary shard size rollover field

15f14f7

large-ish refactor of the hot phase component to move the rollover fi…

640c945

…elds to their own components

added comment

fdd4f1f

jloleysens added release_note:enhancement Feature:ILM v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 labels Apr 8, 2021

jloleysens requested a review from sebelga April 8, 2021 10:31

jloleysens added 3 commits April 8, 2021 14:21

address i18n issue

fbf9027

- fixed jest tests

faca1e7

- fixed behaviour to show missing value for rollover - updated jest snapshots

added field deprecation component

6d2977c

jloleysens requested a review from mdefazio April 12, 2021 12:21

added test for whether deprecation icon is visible

70a76dd

Merge branch 'master' into ilm/support-max_primary_shard_size

5a9f9b3

jloleysens marked this pull request as ready for review April 12, 2021 14:14

jloleysens requested review from a team as code owners April 12, 2021 14:14

sebelga approved these changes Apr 13, 2021

View reviewed changes

jloleysens added 3 commits April 14, 2021 11:53

remove unused import, remove type generic from FormSchema

fdd9abb

fixed getting "getFields" from incorrect object

7d4e546

ryankeairns reviewed Apr 14, 2021

View reviewed changes

ryankeairns requested changes Apr 14, 2021

View reviewed changes

wip!!!

c8f5363

debadair suggested changes Apr 14, 2021

View reviewed changes

jloleysens added 2 commits April 15, 2021 10:44

- removed FieldDeprecationWarning component and associated tests

9764703

- implmented always showing the warning icon and updated the color - cleaned up test helpers

- more tightly grouped the numeric inputs with their unit selects

c93b73c

by reducing the gutter size before defazios feedback - fixed alignment issue between numeric rollover inputs and their unit select field by adding flex-end styling to the groups

mdefazio approved these changes Apr 15, 2021

View reviewed changes

jloleysens merged commit 5ecf098 into elastic:master Apr 15, 2021

jloleysens deleted the ilm/support-max_primary_shard_size branch April 15, 2021 16:02

jloleysens mentioned this pull request Apr 15, 2021

[7.x] [ILM] Support max primary shard size rollover field (#96545) #97283

Merged

jloleysens mentioned this pull request Apr 22, 2021

wait for rollup in ILM #74719

Closed

kruskall mentioned this pull request Sep 27, 2023

Switch built-in ILM rollover policies from max_size to max_primary_shard_size elastic/apm-server#11729

Merged

1 task

This was referenced Sep 27, 2023

Switch fleet's built-in ILM policies to use .actions.rollover.max_primary_shard_size elastic/elasticsearch#99983

Closed

Switch fleet's built-in ILM policies to use .actions.rollover.max_primary_shard_size elastic/elasticsearch#99984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Support max primary shard size rollover field #96545

[ILM] Support max primary shard size rollover field #96545

jloleysens commented Apr 8, 2021 •

edited

Loading

jloleysens commented Apr 12, 2021

elasticmachine commented Apr 12, 2021

sebelga left a comment

sebelga Apr 13, 2021

sebelga Apr 13, 2021

sebelga Apr 13, 2021

ryankeairns Apr 14, 2021

ryankeairns Apr 14, 2021

jloleysens Apr 14, 2021

ryankeairns Apr 14, 2021

ryankeairns left a comment

ryankeairns Apr 14, 2021

cjcenizal commented Apr 14, 2021

debadair left a comment

debadair Apr 14, 2021

debadair Apr 14, 2021

mdefazio commented Apr 14, 2021

cjcenizal commented Apr 14, 2021

jloleysens commented Apr 15, 2021

sebelga commented Apr 15, 2021

kibanamachine commented Apr 15, 2021

mdefazio left a comment

jloleysens commented Apr 15, 2021

	'Maximum index size is deprecated and will be removed in future versions of the Elastic stack. Please use maximum primary shard size instead.',
	'Maximum index size is deprecated and will be removed in a future version. Use maximum primary shard size instead.',

	defaultMessage="Rollover when an index is 30 days old or the primary shard reaches 50 gigabytes."
	defaultMessage="Roll over when an index is 30 days old or any primary shard reaches 50 gigabytes."

[ILM] Support max primary shard size rollover field #96545

[ILM] Support max primary shard size rollover field #96545

Conversation

jloleysens commented Apr 8, 2021 • edited Loading

Summary

Screenshots

How to test

For copy review

Release note

Checklist

jloleysens commented Apr 12, 2021

elasticmachine commented Apr 12, 2021

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Apr 14, 2021

debadair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdefazio commented Apr 14, 2021

cjcenizal commented Apr 14, 2021

jloleysens commented Apr 15, 2021

sebelga commented Apr 15, 2021

kibanamachine commented Apr 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

mdefazio left a comment

Choose a reason for hiding this comment

jloleysens commented Apr 15, 2021

jloleysens commented Apr 8, 2021 •

edited

Loading