feature: Remove logback dependency by implementing native log rotation#321
feature: Remove logback dependency by implementing native log rotation#321
Conversation
Replace logback-core based log rotation with a custom Java standard library implementation to reduce external dependencies. The new implementation provides the same functionality: - Time-based (daily) log rotation - Size-based rotation when maxSizeInBytes is exceeded - Gzip compression of rotated files - Automatic cleanup of old files based on maxNumberOfFiles - Temp file recovery for orphaned files
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the logging infrastructure by replacing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully replaces the logback-core dependency with a custom log rotation implementation using the Java standard library. The new implementation is well-structured and covers the intended features like time-based and size-based rotation, compression, and cleanup. My review focuses on improving the robustness of the new implementation. I've identified a critical issue with how the date is determined for existing log files, which would break daily rotation on application restarts. Additionally, there are a couple of medium-severity issues regarding silent error handling during the rotation process. Addressing these points will make the new log rotation handler more reliable.
| val append = logFile.exists() | ||
| currentFileSize = if append then logFile.length() else 0L | ||
| currentDate = LocalDate.now() |
There was a problem hiding this comment.
There is a bug in the log file initialization logic. When appending to an existing log file (e.g., after an application restart), currentDate is always set to LocalDate.now(). This will prevent daily rotation from triggering correctly if the application restarts after midnight, as it will continue to append to the previous day's log file instead of rotating it.
To fix this, currentDate should be determined from the last modified timestamp of the existing log file. This ensures that if the log file is from a previous day, it will be rotated on the first new log message.
Please also add the necessary imports: import java.time.Instant and import java.time.ZoneId.
val append = logFile.exists()
if (append) {
currentFileSize = logFile.length()
currentDate = java.time.Instant.ofEpochMilli(logFile.lastModified()).atZone(java.time.ZoneId.systemDefault()).toLocalDate()
} else {
currentFileSize = 0L
currentDate = LocalDate.now()
}| private def rotate(): Unit = | ||
| // Close current writer | ||
| if writer != null then | ||
| Try(writer.close()) |
There was a problem hiding this comment.
The Try(writer.close()) call currently swallows any exceptions. If closing the writer fails, it could be because the buffered data couldn't be flushed to disk, leading to silent data loss. It's important to handle this potential failure and report an error, similar to how it's handled in the close() method of the handler for consistency and robustness.
Try(writer.close()) match {
case Failure(e) =>
reportError(null, toException(e), ErrorManager.CLOSE_FAILURE)
case Success(_) =>
// Closing succeeded
}| if logFile.renameTo(rotatedFile) then | ||
| // Compress the rotated file | ||
| compressFile(rotatedFile) |
There was a problem hiding this comment.
The logFile.renameTo(rotatedFile) operation can fail due to various reasons like file permissions or locks from other processes. Currently, such a failure is silent. This means the log file won't be rotated and will continue to grow, while rotation will be re-attempted on every new log message. To aid in diagnosing such issues, it would be beneficial to report an error when the rename operation fails.
if logFile.renameTo(rotatedFile) then
// Compress the rotated file
compressFile(rotatedFile)
else
reportError(s"Failed to rename ${logFile} to ${rotatedFile}", null, ErrorManager.GENERIC_FAILURE)
Fix compilation errors by adding explicit [File] type parameter to Array.empty calls in LogRotationHandler. Also apply scalafmt formatting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use file's last modified date for currentDate when appending to existing log file, ensuring proper daily rotation after application restart - Handle writer.close() errors in rotate() method - Report error when file rename fails during rotation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully removes the logback dependency and replaces it with a custom, native Java implementation for log rotation. The new implementation covers time-based (daily) and size-based rotation, Gzip compression, and automatic cleanup of old files, aligning with the stated goals of reducing external dependencies while maintaining functionality. The changes in build.sbt correctly reflect the dependency removal. The new LogRotationHandler class introduces a comprehensive custom solution for log management.
| s"${new File(fileNameStem).getName}-${dateStr}\\.(\\d+)${java | ||
| .util | ||
| .regex | ||
| .Pattern | ||
| .quote(logFileExt)}\\.gz".r |
There was a problem hiding this comment.
The fileNameStem (or rather, its getName() part) is directly embedded into the regex pattern without being escaped. If fileNameStem contains characters that are special in regular expressions (e.g., ., *, +, ?), this could lead to incorrect matching of log files during findNextIndex. For example, if fileNameStem is my.app.log, the . characters would be treated as wildcards, potentially matching unintended files. It's best practice to quote any part of a string that is dynamically inserted into a regex pattern to ensure it's treated literally.
s"${java.util.regex.Pattern.quote(new File(fileNameStem).getName)}-${dateStr}\\.(\\d+)${java
.util
.regex
.Pattern
.quote(logFileExt)}\\.gz".rQuote the fileNameStem when building regex pattern to handle filenames with special regex characters (e.g., dots in "my.app.log"). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Replace logback-core based log rotation with a custom Java standard library
implementation to reduce external dependencies. The new implementation
provides the same functionality: