GET /_migration/deprecations doesn't check disk watermarks against correct settings values #138115
Conversation
alexey-ivanov-es
added
>bug
:Core/Infra/Core
elasticsearchmachine
added
v9.2.2
v8.19.8
v9.1.8
v9.3.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @alexey-ivanov-es, I've created a changelog YAML for you. |
...cation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java
Show resolved
Hide resolved
|
sorry, i got distracted... I'll get back to this tomorrow morning @alexey-ivanov-es |
mosche
left a comment
mosche
left a comment
There was a problem hiding this comment.
Looking good, just a few minor nits. But wondering if we should address the testing gap here?
...cation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java
Show resolved
Hide resolved
...cation/src/main/java/org/elasticsearch/xpack/deprecation/TransportDeprecationInfoAction.java
Outdated
Show resolved
Hide resolved
| public List<DeprecationIssue> nodeSettingsIssues() { | ||
| return nodeSettingsIssues.get(); | ||
| DeprecationIssue watermarkIssue = diskWatermarkIssue.get(); | ||
| List<DeprecationIssue> deprecationIssues = nodeSettingsIssues.get(); |
There was a problem hiding this comment.
optional nit, how about better distinguishing between the enhanced nodeSettingsIssues and the ones collected from remote nodes, e.g. renaming the latter to remoteNodeSettingsIssues or similar making it more obvious that it's not the same as nodeSettingsIssues that are ultimately returned
| assertThat(exception.getCause().getMessage(), containsString("boom")); | ||
| } | ||
|
|
||
| public void testCheckDiskLowWatermark() { |
There was a problem hiding this comment.
Looks like masterOperation operation isn't covered at all, but obviously that's not new to this PR.
Should that be added? wdyt?
There was a problem hiding this comment.
I've added a unit test for it. I wanted to add an integration or REST test, but found it's quite difficult to set up the behavior needed I wanted to test
3 participants
This change moves the disk watermark check from all nodes to the master node since the check is performed against information in
ClusterInfo. Also, it changes what setting values are used - using values from the cluster state if they are set, or from node settings otherwise.There's a potential edge case: in a mixed cluster (e.g. master 9.2.1 (before the fix), other nodes 9.3 (after the fix)), the check wouldn't be performed. However, due to #137004, the check isn't working in the released versions anyway, so this doesn't make things worse.
Closes #137005