MINOR: conditionally log "Sink record with view" traces#896
MINOR: conditionally log "Sink record with view" traces#896James Yuzawa (yuzawa-san) wants to merge 1 commit into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a performance issue where expensive log message assembly was occurring regardless of the configured log level. The solution wraps trace log statements with isTraceEnabled() checks to prevent unnecessary string conversion operations.
Key Changes:
- Added
isTraceEnabled()guards around trace logging statements in four record writer providers - Prevents the expensive
sinkRecordToLoggableString(record)method from being called when trace logging is disabled
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ParquetRecordWriterProvider.java | Added isTraceEnabled() guard to prevent unnecessary trace log message assembly |
| JsonRecordWriterProvider.java | Added isTraceEnabled() guard to prevent unnecessary trace log message assembly |
| ByteArrayRecordWriterProvider.java | Added isTraceEnabled() guard to prevent unnecessary trace log message assembly |
| AvroRecordWriterProvider.java | Added isTraceEnabled() guard to prevent unnecessary trace log message assembly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
could you please take a look at this? |
1 similar comment
|
could you please take a look at this? |
Problem
there is a memory hotspot here. the log message is assembled regardless the log level.
Solution
solution is to gate it by checking isTraceEnabled()
Does this solution apply anywhere else?
If yes, where?
Test Strategy
check flamecharts again
Testing done:
Release Plan
minor