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

[ISSUE-112] Add total_shards_per_node setting to allocate in ILM #120

Merged
merged 9 commits into from
Nov 10, 2022

Conversation

RobsonSutton
Copy link
Contributor

@RobsonSutton RobsonSutton commented Jun 10, 2022

Fixes #112

@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@RobsonSutton RobsonSutton marked this pull request as ready for review July 7, 2022 18:27
@RobsonSutton
Copy link
Contributor Author

@Kushmaro / @olksdr - Hi both 👋 Please can you review and provide some feedback on this one when you get a chance?

P.S. New to Go so apologies if anything has been missed...!

@tobio
Copy link
Member

tobio commented Jul 25, 2022

jenkins test this

@tobio
Copy link
Member

tobio commented Jul 28, 2022

jenkins test this

@tobio
Copy link
Member

tobio commented Jul 28, 2022

@RobsonSutton the CI test suite is run against a range of ES versions (you can see the pipeline definition here).

I think we'll want to only define this property in the acceptance tests when they're being run against a supported ES version.

@RobsonSutton
Copy link
Contributor Author

@tobio - Apologies for the delay! Yeah I didn't have time the other day to figure this one out but hopefully I should get some time later today to try figure out the best way to skip these for specific versions within the tests 👍

@tobio
Copy link
Member

tobio commented Sep 25, 2022

jenkins test this

1 similar comment
@tobio
Copy link
Member

tobio commented Sep 25, 2022

jenkins test this

@othmane399
Copy link

Hello, did you need some help on this ?

@tobio
Copy link
Member

tobio commented Oct 4, 2022

@othmane399 definitely happy to get some help to get this one across the line.

Bit of a brain dump below, but totally happy if you wanted to take another approach.

I was trying to have the provider only specify total_shards_per_node when it's in module definition, but that's difficult to do without dropping into the raw config objects which I'd rather avoid.

I was going to instead validate the ES version the provider was running against and error out if the property is not supported and not the default.

@RobsonSutton
Copy link
Contributor Author

@tobio - Apologies, have been a bit MIA recently with this one, i'll take another look at this but it may be a bit out of my comfort zone since I'm still picking up the basics currently!

@othmane399
Copy link

othmane399 commented Oct 17, 2022

Hello @tobio, after thinking a bit on this, I think IMO testing pipelines should only run tests files that are version compatible with.
In a draft, let's say as there's a major change on ILM since v7.16, we should have 2 test_ilm.go files, one like test_ilm_before_7.16 and another one test_ilm_after_7.16. and the run choose the corresponding test file related to the ES tested version.
This approach will maybe duplicate testing code at some points but will facilitate the resolution of this kind of issues for all incoming features on this or another resources.
I'm not a go expert so I cannot say If this is easy to implement or not, so this is just an idea :)

@tobio tobio force-pushed the issue-112 branch 3 times, most recently from 31edfe2 to c76f845 Compare November 10, 2022 05:02
Copy link

@webfella webfella left a comment

Choose a reason for hiding this comment

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

LGTM 💥

Comment on lines +512 to +520
var ilmActionSettingOptions = map[string]struct {
skipEmptyCheck bool
def interface{}
minVersion *version.Version
}{
"number_of_replicas": {skipEmptyCheck: true},
"total_shards_per_node": {skipEmptyCheck: true, def: -1, minVersion: version.Must(version.NewVersion("7.16.0"))},
"priority": {skipEmptyCheck: true},
}

Choose a reason for hiding this comment

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

👍

@tobio tobio merged commit d150b45 into elastic:main Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants