refactor(cli): use SLF4J parameterized logging instead of string concatenation#19159
Conversation
…atenation
Convert log string concatenation to {} placeholders across hudi-cli.
Mechanical and behavior-preserving. Notable handling:
- MetadataCommand: two long multi-line concatenations (FS/metadata file size
mismatch) rewritten as wrapped parameterized calls; the inline equality
expression becomes a boolean {} arg.
- RepairsCommand: drop redundant hoodieInstant.toString() in the parameterized arg.
- TableCommand: kept schema.toString(true) as-is (formatted print, not a
redundant no-arg toString).
voonhous
changed the title
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR mechanically converts string-concatenation log calls to SLF4J parameterized ({}) logging across several hudi-cli command classes. I verified placeholder/argument counts and the trailing-Throwable handling in each converted call, and they all look correct and behavior-preserving. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Mostly a clean mechanical refactor; one spot in TableCommand where the parameterized logging conversion doesn't fully achieve its intent.
cc @yihua
Extract schema.toString(true) to a local in the outputFilePath branch so it is computed once and reused for both the log statement and writeToFile, instead of serializing the schema twice. Per review.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR mechanically converts string-concatenation log calls to SLF4J {} parameterized logging across hudi-cli. The placeholder/argument counts and trailing throwable args all check out. While tracing the rewritten MetadataCommand size-mismatch statement, I noticed one likely pre-existing bug in the logged "FS size" value that's worth double-checking in the inline comment. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
| entry.getKey(), | ||
| entry.getValue().getLength() == pathInfoMap.get(entry.getKey()).getLength(), | ||
| entry.getValue().getLength(), | ||
| metadataPathInfoMap.get(entry.getKey()).getLength()); |
There was a problem hiding this comment.
🤖 Since you're rewriting this statement anyway — in this loop entry comes from metadataPathInfoMap, so metadataPathInfoMap.get(entry.getKey()).getLength() is just entry.getValue().getLength() again (the metadata size). The FS size {} arg looks like it should be pathInfoMap.get(entry.getKey()).getLength() instead, matching the equality check on the line above. Could you confirm?
4 participants
Describe the issue this Pull Request addresses
A number of
logcalls inhudi-clibuild their message with+string concatenation. This eagerly builds the string even when the level is disabled and reads worse than a parameterized statement. This PR converts them to SLF4J{}templating. Part of the ongoing logging cleanup (follows #19155, #19156, #19157, #19158).Summary and Changelog
logcalls to{}placeholders acrosshudi-clicommand classes. Mechanical and behavior-preserving.MetadataCommand: two long multi-line concatenations (FS / metadata file-size mismatch) rewritten as wrapped parameterized calls; the inline==equality becomes a boolean{}arg.RepairsCommand: dropped a redundanthoodieInstant.toString()in the parameterized arg.TableCommand: keptschema.toString(true)as-is - it is a formatted print, not a redundant no-argtoString().LockAuditingCommand: the trailingestays the throwable arg, so the stacktrace is logged as before.Impact
none - purely mechanical, behavior-preserving logging change.
Risk Level
none
Documentation Update
none
Contributor's checklist