refactor(client): use SLF4J parameterized logging instead of string concatenation#19155
Conversation
voonhous
changed the title
voonhous
force-pushed
the
cleanup-slf4j-log-templating-hudi-client
branch
from
93db838 to
16ded7c
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR mechanically converts +-concatenated log messages to SLF4J parameterized {} logging across the hudi-client modules, with placeholder counts matching arguments and trailing throwables preserved so stack traces are emitted unchanged. 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. A few spots still call .toString() on the argument being passed to a {} placeholder, which partially defeats the lazy-evaluation benefit this PR is restoring — otherwise the changes look clean.
cc @yihua
With {} templating SLF4J calls toString() lazily on the argument, so the
explicit calls were redundant. Removed in TimelineArchiverV1,
HoodieCompactionPlanGenerator and the matching TestLegacyArchivedMetaEntryReader
log. Addresses review nits on apache#19155.
Claude-Session: https://claude.ai/code/session_01FpDcDnw5gVVKBfPeEqJ3Fk
With {} templating SLF4J calls toString() lazily on the argument, so the
explicit calls were redundant. Removed in TimelineArchiverV1,
HoodieCompactionPlanGenerator and the matching TestLegacyArchivedMetaEntryReader
log. Addresses review nits on apache#19155.
voonhous
force-pushed
the
cleanup-slf4j-log-templating-hudi-client
branch
from
155f39e to
c8a0145
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR converts string-concatenated log statements to SLF4J parameterized {} logging across the hudi-client modules, and the traced call sites show placeholder counts matching their arguments with trailing throwables preserved for stack traces. 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. The conversion from string concatenation to SLF4J parameterized logging is clean throughout — one spot in HoodieBinaryCopyHandle where collapsing two lines into one made the log statement unwieldy.
cc @yihua
…oncatenation
Convert `+`-concatenated log messages to SLF4J `{}` placeholders across
hudi-client (client-common, java-client, spark-client, flink-client).
The rewrite is behaviour-preserving: each concatenated expression becomes a
placeholder argument in order, and a trailing throwable is kept as the last
argument so its stack trace is still recorded. This also avoids building the
message string eagerly when the log level is disabled.
With {} templating SLF4J calls toString() lazily on the argument, so the
explicit calls were redundant. Removed in TimelineArchiverV1,
HoodieCompactionPlanGenerator and the matching TestLegacyArchivedMetaEntryReader
log. Addresses review nits on apache#19155.
voonhous
force-pushed
the
cleanup-slf4j-log-templating-hudi-client
branch
from
c8a0145 to
38ebb72
Compare
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 +-concatenated log messages to SLF4J parameterized {} logging across the hudi-client modules, and the conversions check out — placeholder counts match their argument counts and trailing Throwable arguments remain unmatched so stack traces are preserved. 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. A few dozen clean mechanical conversions — just one spot where collapsing a multi-line concat into a single long parameterized call hurt readability.
cc @yihua
Keep the format string and its arguments on separate lines in HoodieBinaryCopyHandle.write() instead of one ~200-char line, per review.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR mechanically converts +-concatenated log statements to SLF4J parameterized {} logging across the hudi-client modules, preserving placeholder/argument counts and trailing-throwable semantics so stack traces still print. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
|
@wombatu-kun Can i get a re-review on this mechanical PR, thanks. |
4 participants
Describe the issue this Pull Request addresses
Many
hudi-clientlog statements build their message with+string concatenation instead of SLF4J{}placeholders. This is the first batch of a broader logging cleanup.Summary and Changelog
Convert
+-concatenated log messages to SLF4J parameterized{}logging acrosshudi-client(client-common,java-client,spark-client,flink-client) - 132 call sites in 67 files.{}message; 3 long ones were re-wrapped to stay within the 200-char line limit.No code was copied. Behaviour-preserving - no functional change.
Impact
None (logging only). Minor perf benefit: the message string is no longer built eagerly when the log level is disabled.
Risk Level
low - mechanical, behaviour-preserving change. Only provably-safe rewrites were applied (leading-arithmetic and brace-collision cases were skipped and handled by hand). Verified with
mvn test-compileon client-common + java-client (JDK 17) andcheckstyle:checkacross all four submodules.Documentation Update
none
Contributor's checklist