-
Notifications
You must be signed in to change notification settings - Fork 25k
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
More useful toString on UpdateSettingsTask #93514
More useful toString on UpdateSettingsTask #93514
Conversation
Today `UpdateSettingsTask#toString` is just the default which is not helpful when it appears in logs. This commit makes it include the affected indices and settings.
Pinging @elastic/es-data-management (Team:Data Management) |
For the record the relevant elasticsearch/server/src/main/java/org/elasticsearch/cluster/ClusterStateTaskExecutor.java Lines 71 to 82 in 73863dd
We only call this if we need the full string description of the tasks for some reason (typically trace logging), otherwise this code is not called. |
|
||
@Override | ||
public String toString() { | ||
return Arrays.toString(indices()) + settings; |
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.
Should it mention anywhere that this is UpdateSettingsClusterStateUpdateRequest
?
I wonder if it might not be immediately obvious what produced the string in case we have many strings from distinct cluster state update tasks.
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.
We also include the source
of the tasks (which is entry.getKey()
here:
elasticsearch/server/src/main/java/org/elasticsearch/cluster/service/BatchSummary.java
Line 34 in a733f42
return tasks.isEmpty() ? entry.getKey() : entry.getKey() + "[" + tasks + "]"; |
For the update-settings tasks this includes the string update-settings
so I think there is no chance of confusion here.
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.
Added a non-blocking suggestion, otherwise LGTM
Today
UpdateSettingsTask#toString
is just the default which is not helpful when it appears in logs. This commit makes it include the affected indices and settings.