-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix log message format bugs #118354
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 log message format bugs #118354
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @joegallo, I've created a changelog YAML for you. |
nielsbauman
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, Joe!
|
Related to #104782, in that with the changing of the seasons, I sometimes get the urge to |
original-brownbear
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 one cleanup suggestion though :)
| // A normally running shard snapshot should be in stage INIT or STARTED. And we know it's not in PAUSING or ABORTED because | ||
| // the ensureNotAborted() call above did not throw. The remaining options don't make sense, if they ever happen. | ||
| logger.error( | ||
| () -> Strings.format( |
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.
This one is kinda nonsense? :) Why use a lambda and String.format here when you're not logging an exception?
I know it was like that before but maybe just remove the format call and lambda? :)
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.
Sure thing, 6a21cc9.
| return (String) asMap.keySet().toArray()[0]; | ||
| } | ||
| logger.warn("--> No matching rollup name for path [%s]", endpoint); | ||
| logger.warn("--> No matching rollup name for path [{}}]", endpoint); |
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.
Just dropping by, typo 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.
YIKES! Great catch, Sam!
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.
samxbr
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
💔 Backport failed
You can use sqren/backport to manually backport by running |
The codebase is very trappy on this, which is unfortunate, but at present we have no way of preventing this from happening (at least in the broadest sense, I think some simple special cases are checked automatically) besides being really careful all the time.
I just happened to notice that this snuck in via #117575, so I'm chasing that PR (hence the wide list of backports). I'm also including @original-brownbear as a reviewer because of the snapshot code.