-
Notifications
You must be signed in to change notification settings - Fork 84
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
Fix xpack.usage ILM actions #3265
Conversation
They're list of strings, not full actions.
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
/** | ||
* @es_quirk output as a millis number in XPack usage stats, which cannot roundtrip with a Duration as it requires a unit. | ||
*/ | ||
min_age?: Duration | long | ||
min_age?: Duration |
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.
It turns out ILM and Xpack usage output different representations for min_age
. It's always a TimeValue, but:
- ILM APIs call getStringRep() which outputs units for all non-negative values
- XPack usage calls getMillis() which returns a long which is why I'm using
DurationValue<UnitMillis>
now.
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.
Did you forgot to actually add the DurationValue part to the type union or is the diff bugged? I only see a removal of the long variant.
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.
DurationValue is in here: https://github.com/elastic/elasticsearch-specification/pull/3265/files#diff-3e3867651e09e047ca056bb1dbcc10583b2128b45b03cd87233d6dda1a2e6e3fR156. I duplicated the Phase
class in the XPack spec since they're actually different.
@@ -152,6 +151,19 @@ export class HealthStatistics extends Base { | |||
invocations: Invocations | |||
} | |||
|
|||
export class Phase { | |||
actions: string[] |
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.
what about the configuration
field just below?
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.
As mentioned in the pull request description, I chose to not implement it yet, it's long and complicated: https://github.com/elastic/elasticsearch/blob/b4e852a54be436d8b8036da0a0ec4a472d44524a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleFeatureSetUsage.java#L228.
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
(cherry picked from commit cf8e0b8)
(cherry picked from commit cf8e0b8)
(cherry picked from commit cf8e0b8) Co-authored-by: Quentin Pradet <[email protected]>
(cherry picked from commit cf8e0b8) Co-authored-by: Quentin Pradet <[email protected]>
I've been trying to understand today why sometimes 2 responses for xpack.usage validate, and sometimes only one. I don't understand it yet, but I have fixed the
Type 'string[]' has no properties in common with type 'IlmActions'.
issue. Before/after:(Plenty of other issues remain, like configurations is totally missing even though it encodes a lot of information since Elasticsearch 7.15.