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.

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 index_codec to forcemerge action in hot and warm phases #78175

Merged
merged 6 commits into from
Oct 6, 2020

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Sep 22, 2020

Summary

Fixes #76907.
This PR adds an index_codec parameter to the forcemerge action in hot and warm phases. The user can either activate this option for higher compression ratio but slower field performance. If left disabled, the default codec is used.
Docs link: https://www.elastic.co/guide/en/elasticsearch/reference/master/ilm-forcemerge.html

Screenshot

Screenshot 2020-10-06 at 13 12 04

Checklist

Delete any items that are not applicable to this PR.

Release Note

We added an option to select a higher compression codec for force merge action in ILM.

@yuliacech yuliacech added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Sep 22, 2020
@yuliacech yuliacech marked this pull request as ready for review September 22, 2020 16:49
@yuliacech yuliacech requested a review from a team as a code owner September 22, 2020 16:49
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @yuliacech ! I tested the code locally and it looks good to me!

I left one comment about adding another test case, but not blocking on that.

@@ -84,6 +87,12 @@ export const warmPhaseFromES = (phaseSerialized?: SerializedWarmPhase): WarmPhas
? actions.shrink.number_of_shards.toString()
: '';
}

if (actions.set_priority) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes, I am guessing this was missing before!

@@ -367,4 +368,135 @@ describe('Policy serialization', () => {
serializePolicy(deserializedPolicy, originalPolicy);
expect(originalPolicy).toEqual(originalClone);
});

test('serialize a policy using "best_compression" codec for forcemerge', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is worth adding a test that asserts our serialisation will remove the "best_compression" setting from a policy that originally has it specified. This will ensure that we do not have a dangling "best_compression" setting.

@yuliacech
Copy link
Contributor Author

Hi @esdocs team, could you please have a look at my wording for the switch label and description for the codec parameter? The docs also mention that the index will be closed and then re-opened if this option is used. Should we put that into UI too?

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@jrodewig jrodewig 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 some suggestions, but the current UI text is good.

Overall, I think we can abstract away things like codec and the compression ratio. Instead, we can focus on the outcome: Better compression of stored fields at the cost of slower performance.

const bestCompressionLabel = i18n.translate(
'xpack.indexLifecycleMgmt.forcemerge.bestCompressionLabel',
{
defaultMessage: 'Enable higher compression ratio codec',
Copy link
Contributor

Choose a reason for hiding this comment

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

Some alternate suggestions in order of preference:

  • Compress stored fields
  • Use higher compression

Comment on lines 108 to 109
defaultMessage="Uses DEFLATE for a higher compression ratio but slower stored fields performance
than the default LZ4 codec."
Copy link
Contributor

Choose a reason for hiding this comment

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

Some alternate suggestions in order of preference:

  • Better compress stored fields at the cost of slower performance.
  • Use higher compression for stored fields at the cost of slower performance.
  • Use the DEFLATE codec for higher compression but slower stored fields performance. If disabled, the default LZ4 codec is used.

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
indexLifecycleManagement 231.7KB 233.1KB +1.5KB

page load bundle size

id before after diff
indexLifecycleManagement 89.9KB 89.9KB +58.0B

History

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

@yuliacech yuliacech merged commit dcb646d into elastic:master Oct 6, 2020
yuliacech added a commit to yuliacech/kibana that referenced this pull request Oct 6, 2020
…astic#78175)

* [ILM] Add index_codec to forcemerge action in hot and warm phases

* [ILM] Fix i18n check errors

* [ILM] Add code review suggestions

* [ILM] Add code review suggestions

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yuliacech added a commit that referenced this pull request Oct 6, 2020
…8175) (#79652)

* [ILM] Add index_codec to forcemerge action in hot and warm phases

* [ILM] Fix i18n check errors

* [ILM] Add code review suggestions

* [ILM] Add code review suggestions

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@yuliacech yuliacech deleted the ilm_forcemerge_codec branch October 7, 2020 12:53
gmmorris added a commit that referenced this pull request Oct 8, 2020
…into feature/task_manager_429

* 'feature/task_manager_429' of github.com:elastic/kibana: (158 commits)
  Add license check to direct package upload handler. (#79653)
  [Ingest Manager] Rename API /api/ingest_manager => /api/fleet (#79193)
  [Security Solution][Resolver] Simplify CopyableField styling and add comments (#79594)
  Fine-tunes ML related text on Metrics UI (#79425)
  [ML] DF Analytics creation wizard: ensure job creation possible when model memory lower than estimate (#79229)
  Add new "Add Data" tutorials (#77237)
  Update APM telemetry docs (#79583)
  Revert "Add support for runtime field types to mappings editor. (#77420)" (#79611)
  Kibana request headers (#79218)
  ensure missing indexPattern error is bubbled up to error callout (#79378)
  Missing space fix (#79585)
  remove duplicate tab states (#79501)
  [data.ui] Lazy load UI components in data plugin. (#78889)
  Add generic type params to search dependency. (#79608)
  [Ingest Manager] Internal action for policy reassign (#78493)
  [ILM] Add index_codec to forcemerge action in hot and warm phases (#78175)
  [Ingest Manager] Update open API spec and add condition to agent upgrade endpoint (#79579)
  [ML] Hide Data Grid column options when histogram charts are enabled. (#79459)
  [Telemetry] Synchronous `setup` and `start` methods (#79457)
  [Observability] Persist time range across apps (#79258)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ILM release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ILM] Add support for "codec" in the force merge
5 participants