Skip to content

refactor(common): use SLF4J parameterized logging instead of string concatenation#19158

Merged
voonhous merged 1 commit into
apache:masterfrom
voonhous:cleanup-slf4j-log-templating-common
Jul 4, 2026
Merged

refactor(common): use SLF4J parameterized logging instead of string concatenation#19158
voonhous merged 1 commit into
apache:masterfrom
voonhous:cleanup-slf4j-log-templating-common

Conversation

@voonhous

@voonhous voonhous commented Jul 3, 2026

Copy link
Copy Markdown
Member

Describe the issue this Pull Request addresses

A number of log/LOG calls in hudi-common build 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).

Summary and Changelog

  • Converted string-concatenation log/LOG calls to {} placeholders across hudi-common (main + tests). Mechanical and behavior-preserving.
  • Manual handling worth calling out:
    • ZookeeperTestService: the caught IOException is passed as e.toString(), not a bare e. Passed as a trailing e, SLF4J would peel it as the stacktrace throwable, leave the placeholder literal, and dump a stack on an intentionally-ignored path (// ignore as this is expected). toString() keeps it an inline message value, preserving the original output.
    • ConfigUtils: dropped a redundant path.toString(); the trailing e stays the throwable arg, so the stacktrace is logged as before.
    • JmxMetricsReporter: one message wraps the value in literal braces; written as {port: {}} so SLF4J fills the inner {} and prints the outer braces literally.
    • RocksDbBasedFileSystemView: one long line wrapped across two lines to stay within the 200-char limit.

Impact

none - purely mechanical, behavior-preserving logging change.

Risk Level

none

Documentation Update

none

Note: a couple of files still use plain LoggerFactory.getLogger LOG fields. Migrating those to the Lombok @Slf4j annotation will be done in a separate round of cleanup sweeps; this PR only covers the concatenation-to-templating conversion.

Contributor's checklist

  • Read through contributor's guide
  • Enough context is provided in the sections above
  • Adequate tests were added if applicable

…oncatenation

Convert log string concatenation to {} placeholders across hudi-common.
Mechanical and behavior-preserving. Notable manual handling:
- ZookeeperTestService: pass e.toString() (not e) so the exception stays an
  inline message value instead of being peeled as the SLF4J stacktrace arg,
  preserving the existing 'ignore as expected' behavior.
- ConfigUtils: drop redundant path.toString() in the parameterized arg.
- JmxMetricsReporter: the message wraps the value in literal braces; written
  as "{port: {}}" so SLF4J fills the inner {} and keeps the outer braces.
@voonhous voonhous changed the title refactor(common): use SLF4J parameterized logging instead of string c… refactor(common): use SLF4J parameterized logging instead of string concatenation Jul 3, 2026
@github-actions github-actions Bot added the size:S PR with lines of changes in (10, 100] label Jul 3, 2026

@hudi-agent hudi-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for the contribution! This PR mechanically converts string-concatenation log/LOG calls to SLF4J {} parameterized logging across hudi-common (main + tests). I traced each changed statement: placeholder counts match argument counts throughout, no arguments are arrays (which SLF4J would format differently from concatenation), and the two throwable-adjacent edge cases are handled correctly — ConfigUtils passes path + e so the stack trace still logs in both SLF4J 1.7.x and 2.0.x, and ZookeeperTestService's e.toString() correctly avoids the bare-Throwable-as-last-arg divergence. 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. One small redundancy in the ZookeeperTestService change; everything else is clean.

cc @yihua

@hudi-bot

hudi-bot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S PR with lines of changes in (10, 100]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants