refactor(flink): use SLF4J parameterized logging instead of string concatenation#19156
Conversation
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 +-concatenated log messages to SLF4J parameterized {} logging across hudi-flink-datasource. I checked placeholder/argument counts and the trailing-throwable handling on the log.error(...) calls — the throwable arguments are correctly left as the last (unmatched) argument so stack traces are preserved, and all multi-placeholder messages line up with their arguments. 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 couple of pre-existing message format oddities worth cleaning up while these lines are being touched.
cc @yihua
With `{}` templating, "Error ?{}" reads oddly (the `?` looks like a stray
ternary fragment). Reword to "Error: {}" in both the clustering job and
compactor graceful-shutdown logs. Addresses review nit on apache#19156.
Claude-Session: https://claude.ai/code/session_01FpDcDnw5gVVKBfPeEqJ3Fk
With `{}` templating, "Error ?{}" reads oddly (the `?` looks like a stray
ternary fragment). Reword to "Error: {}" in both the clustering job and
compactor graceful-shutdown logs. Addresses review nit on apache#19156.
voonhous
force-pushed
the
cleanup-slf4j-log-templating-flink-datasource
branch
from
49244c9 to
28451c4
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 Flink log statements to SLF4J parameterized {} logging across 22 files. I traced the placeholder-to-argument counts and the trailing-throwable cases (the common SLF4J pitfall) and they all line up correctly — the throwables remain trailing args so stack traces are preserved. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
…ncatenation
Convert `+`-concatenated log messages to SLF4J `{}` placeholders across
hudi-flink-datasource (hudi-flink and the flink1.18.x-2.1.x version modules).
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, "Error ?{}" reads oddly (the `?` looks like a stray
ternary fragment). Reword to "Error: {}" in both the clustering job and
compactor graceful-shutdown logs. Addresses review nit on apache#19156.
voonhous
force-pushed
the
cleanup-slf4j-log-templating-flink-datasource
branch
from
28451c4 to
546bf01
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 Flink log statements to SLF4J parameterized {} logging across 22 files. I traced each converted site, paying particular attention to placeholder/argument counts and the error-with-throwable cases (where the trailing Throwable must remain unmatched by a placeholder so SLF4J logs the stack trace) — those all check out. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
4 participants
Describe the issue this Pull Request addresses
Many
hudi-flink-datasourcelog statements build their message with+string concatenation instead of SLF4J{}placeholders. Follow-up to #19155 (same cleanup forhudi-client).Summary and Changelog
Convert
+-concatenated log messages to SLF4J parameterized{}logging acrosshudi-flink-datasource- 34 call sites in 22 files (hudi-flinkplus theflink1.18.x-2.1.xversion modules).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, identical in shape to the compile-verified
hudi-clientbatch (#19155). Verified here withcheckstyle:checkonhudi-flink; no line exceeds the 200-char limit.Documentation Update
none
Contributor's checklist