-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add to HealthMetadata information about ShardLimits #94116
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
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @HiDAl, I've created a changelog YAML for you. |
|
@elasticsearchmachine run elasticsearch-ci/bwc |
|
BWC tests are not passing due to this: converting to a draft in order of fix the issue first |
4c1e61e to
a07e5be
Compare
|
@elasticsearchmachine run elasticsearch-ci/bwc |
andreidan
left a comment
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 Pablo, this generally looks great.
I left a few suggestions
server/src/internalClusterTest/java/org/elasticsearch/health/HealthMetadataServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/health/HealthMetadataServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadataService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadataService.java
Outdated
Show resolved
Hide resolved
gmarouli
left a comment
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.
Thank for working on this, it's nice to see the HealthMetadata being extended :). I left some minor comments and I second Andrei's comments :).
server/src/internalClusterTest/java/org/elasticsearch/health/HealthMetadataServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/metadata/HealthMetadataSerializationTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/metadata/HealthMetadataSerializationTests.java
Outdated
Show resolved
Hide resolved
9468d2f to
68fb6e2
Compare
|
|
|
@elasticmachine run elasticsearch-ci/bwc |
andreidan
left a comment
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.
LGTM, thanks for iterating on this Pablo
|
@elasticmachine update branch |
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadata.java
Outdated
Show resolved
Hide resolved
fix typo on method/variable names
server/src/main/java/org/elasticsearch/health/metadata/HealthMetadata.java
Outdated
Show resolved
Hide resolved
ShardLimits validation shouldn't run as part of the construction of the object, otherwise it will block the serialization path.
when serializing the value to the wire, we ensure that it's writable/readable using Optional methods. add proper tests to check the null case
|
executed the serialization test 1000 times, no errors: ➜ elasticsearch git:(new-shard-limit-metadata) ✗ ./gradlew ':server:test' --tests "org.elasticsearch.health.metadata.HealthMetadataSerializationTests" -Dtests.iters=1000
=======================================
...
=======================================
> Task :server:test
[...]
BUILD SUCCESSFUL in 18s
61 actionable tasks: 2 executed, 59 up-to-date
Publishing build scan...
https://gradle-enterprise.elastic.co/s/endahupjxcp4y |
|
@elasticmachine run elasticsearch-ci/part-2 |
create all the machinery to have the shard limits information from the master node. Later, these values will be used by the new shard limits health indicator.
ShardLimitsMetadataobject to be filled up by the master node and used by the designedhealth-nodeissue #91119