feat: Add ClpIrFileAppender, AbstractBufferedRollingFileAppender, and AbstractClpIrBufferedRollingFileAppender. #8
Conversation
p1halani
changed the title
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (11)
src/main/java/com/yscope/logging/log4j2/SystemTimeSource.java (3)
3-4: Enhance class documentationThe current documentation is quite brief. Consider expanding it to include:
- Purpose of this implementation
- Behaviour of methods (especially the unsupported setter)
- Usage examples or scenarios
-/** Time source for the system's current time. */ +/** + * Implementation of TimeSource that provides access to system time. + * This implementation provides read-only access to system time and does not support + * time manipulation. + * + * @see TimeSource + */
5-8: Add method documentationAdd Javadoc to clarify the method's behaviour and document the exception.
+ /** + * This operation is not supported as system time cannot be modified. + * + * @param currentTimeInMilliseconds The time value to set (ignored) + * @throws UnsupportedOperationException always + */ @Override public void setCurrentTimeInMilliseconds(long currentTimeInMilliseconds) {
10-11: Add method documentation and consider monotonic time
- Add Javadoc to describe the method's behaviour
- Consider using System.nanoTime() for scenarios requiring monotonic time measurements
+ /** + * Returns the current system time in milliseconds. + * + * @return The current time in milliseconds since the epoch + */ @Override public long getCurrentTimeInMilliseconds() { return System.currentTimeMillis(); }Note: If this time source is used for measuring intervals (e.g., for rolling files), consider providing an alternative implementation using
System.nanoTime()to handle system clock adjustments gracefully.src/main/java/com/yscope/logging/log4j2/TimeSource.java (1)
1-18: The TimeSource interface looks good, with minor documentation suggestions.The interface is well-designed for its purpose of facilitating time source abstraction in testing scenarios. Consider these documentation enhancements:
- Add
@throws UnsupportedOperationExceptiontosetCurrentTimeInMillisecondsdocumentation- Add
@sincetags for API versioning- Consider documenting behaviour for negative time values
/** * Interface for a time source. This interface is currently used to swap out real time sources * (e.g., System.currentTimeMillis()) with manual time sources that can be used during unit tests. + * @since 1.0.0 */ public interface TimeSource { /** * If supported, sets the current time for this time source. * * @param currentTimeInMilliseconds Current time in milliseconds from some fixed point in time * (e.g. the UNIX epoch). + * @throws UnsupportedOperationException if the time source doesn't support setting time */ public void setCurrentTimeInMilliseconds(long currentTimeInMilliseconds); - /** @return The time source's current time */ + /** + * @return The time source's current time in milliseconds since epoch + * @since 1.0.0 + */ public long getCurrentTimeInMilliseconds(); }src/main/java/com/yscope/logging/log4j2/Utils.java (1)
10-17: Enhance method documentation for clarity and completeness.While the current documentation is good, consider adding:
- Thread safety guarantees
- Behaviour when parent path is null
- @SInCE tag for API versioning
/** * Creates and opens an output file and file output stream. * * @param filePath Path for the file * @return The file output stream * @throws IOException on I/O error + * @throws IOException if the parent path is null + * @implNote This method is thread-safe for different file paths but concurrent calls + * with the same file path may result in race conditions + * @since 1.0.0 */pom.xml (2)
43-43: Consider updating zstd-jni versionThe specified version (1.5.2-2) is somewhat dated. Consider upgrading to the latest stable version (1.5.5-11) for potential performance improvements and bug fixes.
47-56: Review dependency versions and scopesA few suggestions for the new dependencies:
- Consider upgrading commons-io to the latest stable version (2.15.1)
- Consider specifying appropriate scopes for both dependencies to prevent unnecessary transitive inclusions
src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java (2)
83-86: Consider limiting the visibility of getCompressionPatternLayout().Since
getCompressionPatternLayout()is intended for debugging purposes only, consider reducing its visibility to package-private or protected to prevent unintended use.Apply this diff to modify the method's visibility:
-public PatternLayout getCompressionPatternLayout() { +PatternLayout getCompressionPatternLayout() { return compressionPatternLayout; }
38-39: Improve JavaDoc wording for formality and clarity.The phrase "we error out immediately" is informal. Consider rephrasing for a more professional tone, such as "an exception is thrown immediately if the timestamp pattern is not found."
Apply this change to the JavaDoc:
* Note: We only support a PatternLayout with a timestamp for now. - * Otherwise, we error out immediately. + * Otherwise, an exception is thrown immediately.src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java (1)
175-175: Remove unused call toevent.getLevel().The call to
event.getLevel();at line 175 appears to have no effect as the result is not used. Consider removing this line to clean up the code.src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java (1)
50-50: Typo in class documentation: duplicated word "had"In the JavaDoc comment, there is a repeated word in the sentence:
"...the soft deadline will be set as if the second log event had a had 5s soft timeout."
Please remove the duplicate "had" for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
pom.xml(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java(1 hunks)src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java(1 hunks)src/main/java/com/yscope/logging/log4j2/SystemTimeSource.java(1 hunks)src/main/java/com/yscope/logging/log4j2/TimeSource.java(1 hunks)src/main/java/com/yscope/logging/log4j2/Utils.java(1 hunks)
🔇 Additional comments (8)
src/main/java/com/yscope/logging/log4j2/TimeSource.java (1)
1-18: Verify consistent implementation across time sources.
Let's ensure all implementations of this interface maintain consistent behaviour.
✅ Verification successful
Let me try a different search pattern to find the implementations.
Let me check the SystemTimeSource implementation in detail.
Implementation consistency verified for TimeSource interface
The codebase contains only one implementation (SystemTimeSource) which follows a consistent and logical pattern:
getCurrentTimeInMilliseconds()returns the system time as expectedsetCurrentTimeInMilliseconds()throwsUnsupportedOperationExceptionwhich is appropriate for a system time source
The interface design allows for test implementations to be added later, while the current system implementation maintains proper behaviour.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of TimeSource interface
# to verify consistent implementation patterns
# Search for TimeSource implementations
echo "Searching for TimeSource implementations..."
ast-grep --pattern 'class $_ implements $$$TimeSource$$$'
# Search for getCurrentTimeInMilliseconds implementations
echo -e "\nSearching for getCurrentTimeInMilliseconds implementations..."
ast-grep --pattern 'getCurrentTimeInMilliseconds() {
$$$
}'
# Search for setCurrentTimeInMilliseconds implementations
echo -e "\nSearching for setCurrentTimeInMilliseconds implementations..."
ast-grep --pattern 'setCurrentTimeInMilliseconds($_) {
$$$
}'
Length of output: 564
Script:
#!/bin/bash
# Search for TimeSource implementations using ripgrep
echo "Searching for TimeSource implementations..."
rg "implements.*TimeSource" -l
# Search for files containing TimeSource-related methods
echo -e "\nSearching for TimeSource-related method implementations..."
rg "getCurrentTimeInMilliseconds|setCurrentTimeInMilliseconds" -A 5
Length of output: 4057
Script:
#!/bin/bash
# Get the full content of SystemTimeSource implementation
echo "Content of SystemTimeSource.java:"
cat src/main/java/com/yscope/logging/log4j2/SystemTimeSource.java
# Search for any test files related to TimeSource
echo -e "\nSearching for TimeSource test files..."
rg -l "TimeSource.*Test|Test.*TimeSource"
Length of output: 674
src/main/java/com/yscope/logging/log4j2/Utils.java (2)
1-9: LGTM! Clean and well-organized imports.
All imports are necessary and properly organized.
18-32: Verify proper resource management in calling code.
Since this method returns a FileOutputStream, ensure all callers properly close the stream.
✅ Verification successful
Let me also search for the class name to ensure we catch any static imports:
And let's check for try-with-resources or close() patterns around FileOutputStream:
Resource management is properly implemented
The ClpIrFileAppender implements proper resource management for the FileOutputStream returned by Utils.createOutputFile():
- The stream is wrapped in a
CountingOutputStreamand then aZstdOutputStream - The appender implements proper cleanup through:
- A
closeStream()method that ensures the stream is closed - A shutdown hook to handle JVM shutdown
- Synchronized
stop()method that callscloseStream() - Proper handling in
startNewFile()which closes the previous stream
- A
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of createOutputFile to verify proper resource management
ast-grep --pattern 'Utils.createOutputFile($_)' -A 10
Length of output: 55
Script:
#!/bin/bash
# Search for usage of createOutputFile using ripgrep for better coverage
rg "createOutputFile" -A 10 --type java
Length of output: 3482
Script:
#!/bin/bash
# Search for the Utils class usage
rg "import.*Utils" --type java
Length of output: 159
Script:
#!/bin/bash
# Search for FileOutputStream usage patterns
rg "FileOutputStream" -B 5 -A 10 --type java
Length of output: 6049
Script:
#!/bin/bash
# Let's check for close/cleanup methods in ClpIrFileAppender
rg "close|cleanup|stop" -B 5 -A 10 src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java
# Also check for any AutoCloseable/Closeable implementations
rg "implements.*Close|extends.*Close" src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java
Length of output: 7254
pom.xml (2)
62-62: LGTM: Appropriate scope specification
Setting the log4j dependencies to 'provided' scope is the correct approach for a library project, as these dependencies should be managed by the consuming application.
Also applies to: 68-68
70-74: LGTM: Verify compression library compatibility
The dependency configuration looks good. The 'provided' scope is appropriate for the compression library.
Let's verify if there are any compatibility requirements between clp-ffi and zstd-jni versions:
✅ Verification successful
LGTM: Zstd compression configuration is properly validated
The code properly validates the compression level against zstd-jni's supported range using Zstd.minCompressionLevel() and Zstd.maxCompressionLevel(). The default compression level is set to 3, which is a balanced choice for compression ratio vs performance. The implementation includes proper error handling for invalid compression levels.
- The
ClpIrFileAppendervalidates compression settings before initialization - The
AbstractClpIrBufferedRollingFileAppenderperforms similar validation in its builder - The zstd-jni library is properly integrated with the compression stream setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version compatibility documentation or requirements
rg -i "zstd|compression" README.md
Length of output: 36
Script:
#!/bin/bash
# Search for zstd usage and version requirements in source files
rg -i "zstd" --type java -A 3 -B 3
# Look for compression-related configuration or constants
rg -i "compression" --type java -A 3 -B 3
# Check if there are any test files related to zstd
fd -e java -e xml -H test.*zstd
Length of output: 32028
src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java (1)
74-79:
Ensure thread safety when using logMsgByteBufferDestination.
If instances of CompressionPatternLayoutContainer are accessed by multiple threads, the logMsgByteBufferDestination may not be thread-safe, potentially leading to concurrency issues. Verify whether this class is intended for use in a multi-threaded environment, and if so, consider synchronizing access or using thread-local instances.
Run the following script to check for concurrent usage:
src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java (2)
59-59: Clarify the value of estimatedFormattedTimestampLength.
The field estimatedFormattedTimestampLength is set to 0, which might not accurately represent the estimated length of the formatted timestamp. If this is intentional, please confirm; otherwise, consider setting it to a reasonable estimate to improve the accuracy of uncompressedSizeInBytes.
305-340: Builder class implementation is well-designed.
The Builder class follows the builder pattern effectively, providing a fluent interface for constructing instances of ClpIrFileAppender.
src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java (1)
7-40: Consider adding thread safety documentation.The documentation is comprehensive regarding the purpose and implementation. However, it would be beneficial to document thread safety considerations, especially since this class manages shared buffer state.
Add a note about thread safety, for example:
* Note: The internal buffer must be cleared before it is reused + * <p> + * Thread Safety: This class is not thread-safe and should be used in a single-threaded context + * or with proper external synchronization. */src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java (1)
780-881: Consider adding parameter validation in Builder setters.The Builder setters accept parameters without validation. Consider adding checks for:
- Non-negative timeout values
- Valid CSV format in flush timeout strings
- Non-null baseName when required
Example validation for timeout:
public B setTimeoutCheckPeriod(int timeoutCheckPeriod) { + if (timeoutCheckPeriod <= 0) { + throw new IllegalArgumentException("Timeout check period must be positive"); + } this.timeoutCheckPeriod = timeoutCheckPeriod; return asBuilder(); }src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (1)
148-149: Eliminate redundantPatternLayoutcheckThe
validateParameters()method in theBuilderclass already ensures thatgetLayout()is an instance ofPatternLayout. SinceactivateOptionsHook()is called after validation, the check at lines 148-149 is redundant and can be removed to simplify the code.Apply this diff to remove the redundant check:
- if (!(getLayout() instanceof PatternLayout)) { - throw new RuntimeException("log4j2-appender currently only supports Pattern layout"); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
pom.xml(2 hunks)spotbugs-exclude.xml(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/Boilerplate.java(0 hunks)src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java(1 hunks)src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java(1 hunks)src/test/java/com/yscope/logging/log4j2/ClpIrFileAppenderTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/yscope/logging/log4j2/Boilerplate.java
🚧 Files skipped from review as they are similar to previous changes (2)
- pom.xml
- src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java
🔇 Additional comments (9)
src/test/java/com/yscope/logging/log4j2/ClpIrFileAppenderTest.java (1)
7-7: Class name properly reflects the component under test.
The class name accurately represents its purpose of testing the ClpIrFileAppender implementation.
spotbugs-exclude.xml (4)
44-48: Custom synchronization needs documentation
The justification mentions custom synchronization for shutdown, but this should be well-documented in the code to prevent future synchronization issues.
Let's verify the synchronization implementation:
#!/bin/bash
# Search for synchronized blocks and shutdown-related methods
ast-grep --pattern 'class AbstractBufferedRollingFileAppender {
$$$
synchronized $_ $_($$$) {
$$$
}
$$$
}'Consider enhancing the justification to explain the synchronization strategy:
- <Justification>Custom synchronization is for shutdown after all user threads</Justification>
+ <Justification>Custom synchronization ensures orderly shutdown by waiting for all user threads to complete logging before closing resources</Justification>20-24: Security implications need verification
While returning a buffer directly might be acceptable in some cases, we should verify that the exposed buffer doesn't contain sensitive information.
Let's check the buffer usage:
#!/bin/bash
# Search for buffer handling in PatternLayoutBufferDestination
ast-grep --pattern 'class PatternLayoutBufferDestination {
$$$
$_ getBuffer() {
$$$
}
$$$
}'26-42: Verify Log4j2 plugin attribute usage
The justification indicates these are Log4j2 plugin attributes, but we should verify they are properly annotated.
Let's check for plugin annotations:
#!/bin/bash
# Search for @PluginAttribute annotations in builder classes
rg -A 1 "@PluginAttribute" 2-19: Constructor throws validation needs more context
The justification "Log4j2 should throw immediately if configurations are wrong" is valid, but it would be helpful to specify which configuration parameters are being validated in these constructors.
Let's verify the constructor implementations:
Consider enhancing the justification with specific configuration parameters being validated, e.g.:
- <Justification>Log4j2 should throw immediately if configurations are wrong</Justification>
+ <Justification>Log4j2 should throw immediately if compression settings or file path configurations are invalid</Justification>✅ Verification successful
SpotBugs exclusions are appropriately justified
Based on the code analysis, the constructors validate critical configuration parameters:
- ClpIrFileAppender: compression level and encoding settings
- CompressionPatternLayoutContainer: pattern layout and timestamp pattern configurations
- AbstractBufferedRollingFileAppender: file handling configurations
The current justification is accurate and sufficient since these are all configuration parameters that must be validated immediately to ensure proper logging functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor implementations in the specified classes
ast-grep --pattern 'class $_ {
$$$
$_($$$) throws $_ {
$$$
}
$$$
}'
Length of output: 402
Script:
#!/bin/bash
# Search for constructor implementations in the specified classes using ripgrep
rg -t java "class (AbstractBufferedRollingFileAppender|ClpIrFileAppender|CompressionPatternLayoutContainer)" -A 10
Length of output: 4401
Script:
#!/bin/bash
# Search for configuration validation in constructors
rg -t java "@Plugin.*class (AbstractBufferedRollingFileAppender|ClpIrFileAppender|CompressionPatternLayoutContainer)" -B 2 -A 20
Length of output: 130
Script:
#!/bin/bash
# Search for @PluginAttribute annotations to identify configuration parameters
rg -t java "@PluginAttribute.*class (AbstractBufferedRollingFileAppender|ClpIrFileAppender|CompressionPatternLayoutContainer)" -A 5
Length of output: 133
Script:
#!/bin/bash
# Search for Plugin annotations and constructor parameters
rg -t java "@Plugin.*name.*ClpIrFileAppender" -A 20
# Search for constructor parameters and validation
rg -t java "private.*ClpIrFileAppender\(" -B 5 -A 15
Length of output: 108
Script:
#!/bin/bash
# Search for builder methods that might contain validation
rg -t java "public static \w+Builder createAppender" -B 2 -A 20
Length of output: 65
Script:
#!/bin/bash
# Search for validation methods
rg -t java "private static void validate" -B 2 -A 15
Length of output: 54
Script:
#!/bin/bash
# Search for Plugin factory methods and their parameters
rg -t java "@Plugin.*name.*\"ClpIrFileAppender\"" -B 2 -A 40
# Search for PluginAttribute annotations to find configuration parameters
rg -t java "@PluginAttribute" -B 2 -A 5 src/main/java/com/yscope/logging/log4j2/
Length of output: 141
Script:
#!/bin/bash
# Search for configuration-related constants and fields
rg -t java "private.*final" src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java
rg -t java "private.*final" src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java
Length of output: 535
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java (2)
1-6: LGTM! Clean and minimal imports.
The package structure and imports are well-organized, including only the necessary dependencies.
41-50: 🛠️ Refactor suggestion
Consider making the buffer field final.
While the buffer reference is only reassigned during capacity increases, marking it as final would prevent accidental reassignment elsewhere. Instead of reassigning the reference, you could use buf.clear() followed by buf.put() to copy the contents.
Consider this alternative implementation:
- private ByteBuffer buf;
+ private final ByteBuffer buf;Then update the drain and writeBytes methods to use buffer operations instead of reference reassignment.
Likely invalid or redundant comment.
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java (2)
78-80: LGTM! Well-structured class hierarchy.
The class properly extends AbstractAppender and implements Flushable, providing a solid foundation for specialized appender implementations.
434-466: LGTM! Robust implementation of core logging functionality.
The append method demonstrates excellent practices:
- Proper synchronization
- Comprehensive error handling
- Clear separation of concerns with the abstract appendHook
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (2)
29-30: Consider making file extension constant privateThe constant
CLP_COMPRESSED_IRSTREAM_FILE_EXTENSIONappears to be used only within this class. Consider making it private to reduce the public API surface area.- public static final String CLP_COMPRESSED_IRSTREAM_FILE_EXTENSION = ".clp.zst"; + private static final String CLP_COMPRESSED_IRSTREAM_FILE_EXTENSION = ".clp.zst";
42-43: Add documentation for size tracking fieldsThese fields track important metrics but lack documentation explaining their purpose and units.
+ // Tracks the total compressed size of all previously rolled-over files in bytes private long compressedSizeSinceLastRollover = 0L; // Bytes + // Tracks the total uncompressed size of all previously rolled-over files in bytes private long uncompressedSizeSinceLastRollover = 0L; // Bytes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java(1 hunks)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java
Outdated
Show resolved
Hide resolved
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (7)
29-30: Consider using a more specific name for the file extension constant.The constant
CLP_COMPRESSED_IRSTREAM_FILE_EXTENSIONis used to specify the file extension for CLP compressed IR-stream files. However, the name doesn't clearly indicate that it includes the compression format (zst).To improve clarity, consider renaming it to something like
CLP_ZSTD_COMPRESSED_IRSTREAM_FILE_EXTENSION. This way, it's immediately apparent that the extension includes the Zstandard compression format.-public static final String CLP_COMPRESSED_IRSTREAM_FILE_EXTENSION = ".clp.zst"; +public static final String CLP_ZSTD_COMPRESSED_IRSTREAM_FILE_EXTENSION = ".clp.zst";
88-90: Simplify the setter method by removing the underscore prefix.In Java, it's not conventional to use underscores as prefixes for method names. The underscore prefix is often used for private or internal methods in some naming conventions, but it's not a widely adopted practice in Java.
To align with Java naming conventions and improve readability, consider removing the underscore prefix from the method name.
-private void _setCloseFrameOnFlush(boolean closeFrameOnFlush) { +private void setCloseFrameOnFlush(boolean closeFrameOnFlush) { this.closeFrameOnFlush = closeFrameOnFlush; }
97-99: Simplify the setter method by removing the underscore prefix.Similar to the previous comment, consider removing the underscore prefix from the method name to align with Java naming conventions and improve readability.
-private void _setCompressionLevel(int compressionLevel) { +private void setCompressionLevel(int compressionLevel) { this.compressionLevel = compressionLevel; }
106-108: Simplify the setter method by removing the underscore prefix.As mentioned earlier, consider removing the underscore prefix from the method name to align with Java naming conventions and improve readability.
-private void _setRolloverCompressedSizeThreshold(long rolloverCompressedSizeThreshold) { +private void setRolloverCompressedSizeThreshold(long rolloverCompressedSizeThreshold) { this.rolloverCompressedSizeThreshold = rolloverCompressedSizeThreshold; }
115-117: Simplify the setter method by removing the underscore prefix.Consistent with the previous comments, consider removing the underscore prefix from the method name to align with Java naming conventions and improve readability.
-private void _setRolloverUncompressedSizeThreshold(long rolloverUncompressedSizeThreshold) { +private void setRolloverUncompressedSizeThreshold(long rolloverUncompressedSizeThreshold) { this.rolloverUncompressedSizeThreshold = rolloverUncompressedSizeThreshold; }
123-125: Simplify the setter method by removing the underscore prefix.As suggested earlier, consider removing the underscore prefix from the method name to align with Java naming conventions and improve readability.
-private void _setUseFourByteEncoding(boolean useFourByteEncoding) { +private void setUseFourByteEncoding(boolean useFourByteEncoding) { this.useFourByteEncoding = useFourByteEncoding; }
128-130: Simplify the setter method by removing the underscore prefix.Consistent with the previous suggestions, consider removing the underscore prefix from the method name to align with Java naming conventions and improve readability.
-private void _setOutputDir(String outputDir) { +private void setOutputDir(String outputDir) { this.outputDir = outputDir; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java(1 hunks)
🔇 Additional comments (14)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (14)
14-25: Excellent class-level documentation!
The class-level Javadoc provides a clear and concise overview of the purpose and functionality of the AbstractClpIrBufferedRollingFileAppender class. It effectively communicates the key aspects, such as:
- Extending
AbstractBufferedRollingFileAppenderfor appending to CLP compressed IR-stream files - Performing rollover based on both uncompressed and compressed data sizes
- Ensuring manageability of decompressed files and optimizing for remote storage
- Handling synchronization in derived classes
The documentation is well-structured, using <p> tags for paragraphs and providing a coherent flow of information.
48-86: LGTM!
The constructor takes all the necessary parameters to initialize the appender, including the base parameters from AbstractBufferedRollingFileAppender and the specific parameters for CLP compression and rollover thresholds.
The constructor properly delegates the base parameters to the superclass constructor and sets the CLP-specific parameters using the private setter methods. This approach encapsulates the initialization logic and promotes code reuse.
133-135: LGTM!
The getUncompressedSize() method correctly returns the total uncompressed size of all log events processed by the appender. It takes into account the uncompressed size since the last rollover (uncompressedSizeSinceLastRollover) and the current uncompressed size from the clpIrFileAppender instance.
138-140: LGTM!
The getCompressedSize() method correctly returns the total compressed size of all log events processed by the appender. Similar to getUncompressedSize(), it considers the compressed size since the last rollover (compressedSizeSinceLastRollover) and the current compressed size from the clpIrFileAppender instance.
145-162: LGTM!
The activateOptionsHook() method is properly overridden to initialize the clpIrFileAppender instance with the necessary parameters. It computes the log file name and path based on the provided baseName and currentTimestamp, and ensures that the layout is an instance of PatternLayout.
The initialization of clpIrFileAppender is done with the appropriate parameters, including the file path, appender name, exception handling, layout, filter, compression level, encoding, and close frame on flush setting.
164-166: LGTM!
The closeHook() method is overridden to properly stop the clpIrFileAppender instance when the appender is closed. It uses the stop() method of clpIrFileAppender with the default stop timeout and time unit.
169-172: LGTM!
The rolloverRequired() method is overridden to check if a rollover is necessary based on the compressed and uncompressed size thresholds. It compares the current compressed and uncompressed sizes obtained from the clpIrFileAppender instance with the respective threshold values (rolloverCompressedSizeThreshold and rolloverUncompressedSizeThreshold).
If either the compressed size or the uncompressed size exceeds its threshold, the method returns true, indicating that a rollover is required.
175-181: LGTM!
The startNewLogFile() method is overridden to handle the creation of a new log file during a rollover. It performs the following steps:
-
Updates the
compressedSizeSinceLastRolloveranduncompressedSizeSinceLastRollovervariables by adding the current compressed and uncompressed sizes from theclpIrFileAppenderinstance. This keeps track of the total sizes since the last rollover. -
Computes the new log file name and path based on the provided
baseNameandlastEventTimestamp. -
Calls the
startNewFile()method of theclpIrFileAppenderinstance with the new file path to start writing to the new log file.
The method effectively manages the rollover process by updating the size trackers and initiating a new log file.
184-186: LGTM!
The appendHook() method is overridden to delegate the appending of log events to the clpIrFileAppender instance. It simply calls the append() method of clpIrFileAppender with the provided LogEvent object.
This allows the actual appending logic to be handled by the ClpIrFileAppender class, promoting separation of concerns and code reuse.
189-191: LGTM!
The flush() method is overridden to delegate the flushing of the appender to the clpIrFileAppender instance. It calls the flush() method of clpIrFileAppender, which ensures that any buffered data is written to the underlying output stream.
This allows the actual flushing logic to be handled by the ClpIrFileAppender class, promoting separation of concerns and code reuse.
194-200: LGTM!
The computeSyncRequestMetadata() method is overridden to include additional metadata specific to the AbstractClpIrBufferedRollingFileAppender in the sync request. It creates a HashMap and adds the following metadata entries:
"compressedLogSize": The current compressed size of the log obtained fromclpIrFileAppender.getCompressedSize()."uncompressedLogSize": The current uncompressed size of the log obtained fromclpIrFileAppender.getUncompressedSize()."numEventsLogged": The number of log events logged obtained fromgetNumEventsLogged().
Including these metadata entries in the sync request provides valuable information about the current state of the appender, such as the compressed and uncompressed log sizes and the number of logged events.
203-205: LGTM!
The computeLogFileName() method is overridden to generate the log file name based on the provided baseName and logRolloverTimestamp. It concatenates the baseName, the logRolloverTimestamp, and the CLP_COMPRESSED_IRSTREAM_FILE_EXTENSION constant to form the complete log file name.
This ensures that each log file has a unique name based on the base name and the timestamp of the rollover event, and includes the appropriate file extension for CLP compressed IR-stream files.
213-215: LGTM!
The computeLogFilePath() method takes a logFileName as input and generates the complete file path for the log file. It uses the Paths.get() method to combine the outputDir and the logFileName into a Path object, and then converts it to a string representation using toString().
This method encapsulates the logic for constructing the log file path based on the configured output directory and the generated log file name, providing a consistent way to determine the location of the log files.
217-322: LGTM!
The Builder class provides a fluent API for configuring and building instances of AbstractClpIrBufferedRollingFileAppender. It extends the AbstractBufferedRollingFileAppender.Builder class and adds builder methods specific to the CLP compression and rollover threshold settings.
The builder class includes:
- Private instance variables for storing the configuration values, annotated with
@PluginBuilderAttributefor automatic configuration via Log4j's plugin system. - Setter methods for each configuration property, allowing fluent chaining of method calls.
- A
validateParameters()method that performs validation checks on the configured values, such as ensuring the compression level is within the valid range, the layout is aPatternLayout, the output directory is specified, and the rollover thresholds are positive.
The builder class encapsulates the construction logic and provides a convenient way to create instances of AbstractClpIrBufferedRollingFileAppender with desired configurations.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
spotbugs-exclude.xml (1)
1-49: Consider adding XML schema validationThe SpotBugs filter configuration would benefit from XML schema validation to ensure correctness.
Add the following schema declaration:
+<?xml version="1.0" encoding="UTF-8"?> +<!DOCTYPE FindBugsFilter PUBLIC + "-//SpotBugs//DTD Filter//EN" + "https://spotbugs.readthedocs.io/en/latest/filter.html"> <FindBugsFilter>src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java (3)
7-40: Add @SInCE tag to class documentation.Consider adding the
@sincetag to track when this class was introduced. This helps with version tracking and API documentation./** * This class is a BufferDestination compatible with Log4j2 version 2.9+ (August 31, 2017). + * @since 1.0.0 */
41-50: Add Javadoc for class fields.Consider adding Javadoc comments for the fields to document their purpose and constraints.
+ /** + * Buffer allocation unit size in bytes, aligned with virtual memory page size. + */ private static final int BUF_ALLOCATION_UNIT = 4096; + + /** + * Default initial size for the log message buffer. + */ private static final int DEFAULT_LOG_MSG_BUF_SIZE = BUF_ALLOCATION_UNIT; + /** + * Internal buffer for storing character-encoded log messages. + * Not meant to be used as a traditional ByteBuffer - no read/write flipping required. + */ private ByteBuffer buf;
115-117: Simplify buffer size calculation.The current calculation for the new buffer size can be simplified to improve readability.
- int newBufSize = BUF_ALLOCATION_UNIT * (int)(Math.ceil( - requiredBufSize / (float)BUF_ALLOCATION_UNIT - )); + int newBufSize = ((requiredBufSize + BUF_ALLOCATION_UNIT - 1) / BUF_ALLOCATION_UNIT) * BUF_ALLOCATION_UNIT;src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (3)
32-47: Consider adding Javadoc for instance variablesThe instance variables would benefit from Javadoc comments explaining their purpose, valid ranges, and units (where applicable). This would improve code maintainability and make it easier for other developers to understand the configuration options.
Add Javadoc comments for each instance variable, for example:
// Appender settings, some of which may be set by Log4j through reflection. - // For descriptions of the properties, see their setters below. + /** Directory where log files will be written */ private String outputDir; + /** Compression level for Zstandard compression (1-22) */ private int compressionLevel;
149-150: Improve boolean comparison readabilityThe
false ==comparison style is unusual in Java. Consider using the more conventional!operator for better readability.- if (false == (getLayout() instanceof PatternLayout)) { + if (!(getLayout() instanceof PatternLayout)) {
221-238: Document rationale for default valuesThe default values for compression parameters would benefit from comments explaining why these specific values were chosen. This helps maintainers understand the reasoning behind these choices.
@PluginBuilderAttribute("CompressionLevel") + // Default compression level of 3 provides a good balance between compression ratio and speed private int compressionLevel = 3; @PluginBuilderAttribute("RolloverCompressedSizeThreshold") + // 16MB compressed size threshold ensures efficient file handling and upload private long rolloverCompressedSizeThreshold = 16 * 1024 * 1024; // Bytes; @PluginBuilderAttribute("RolloverUncompressedSizeThreshold") + // 1GB uncompressed size threshold keeps files manageable when decompressed private long rolloverUncompressedSizeThreshold = 1024L * 1024 * 1024; // Bytes;src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java (2)
58-71: Add field documentationCritical fields lack documentation explaining their purpose and invariants. This makes it harder for maintainers to understand the class's internal state management.
Consider adding Javadoc comments for important fields:
+ /** Size of uncompressed data written to current file */ private long uncompressedSizeInBytes = 0; + /** Stream for writing CLP IR format data */ private AbstractClpIrOutputStream clpIrOutputStream; + /** Stream for tracking compressed data size */ private CountingOutputStream countingOutputStream; + /** Flag indicating if appender has been activated through Log4j2 */ private boolean activated = false;
299-301: Use StandardCharsets.UTF_8 instead of ISO-8859-1The code uses ISO-8859-1 encoding for timestamp pattern bytes. UTF-8 is generally preferred as it's the de facto standard for text encoding and handles all Unicode characters correctly.
uncompressedSizeInBytes += compressionLayoutContainer.getTimestampPattern().getBytes( - StandardCharsets.ISO_8859_1 + StandardCharsets.UTF_8 ).length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
spotbugs-exclude.xml(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java(1 hunks)src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java(1 hunks)src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java
- src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (1)
Learnt from: jackluo923
PR: y-scope/log4j2-appenders#8
File: src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java:213-215
Timestamp: 2024-11-01T11:44:55.445Z
Learning: In the `computeLogFilePath` method of `AbstractClpIrBufferedRollingFileAppender.java`, path normalization and traversal checks are unnecessary.
src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java (1)
Learnt from: jackluo923
PR: y-scope/log4j2-appenders#8
File: src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java:113-129
Timestamp: 2024-11-01T09:22:24.215Z
Learning: In `PatternLayoutBufferDestination.java`, additional parameter validation in the `writeBytes(byte[] data, int offset, int length)` method is unnecessary because the Log4j2 library already performs these checks outside this method.
🔇 Additional comments (6)
spotbugs-exclude.xml (4)
20-24: Review security implications of exposed buffer
While returning the buffer directly might be necessary for performance, we should ensure:
- The buffer cannot be modified by malicious code
- No sensitive information is exposed
✅ Verification successful
Buffer exposure is safe and intentionally designed for performance
The SpotBugs exclusion is justified based on the code analysis:
- The buffer is used specifically for Log4j2's internal performance optimization to avoid garbage collection overhead
- The buffer is managed within a controlled context:
- It's used as a pre-allocated space for Log4j2's StringBuilderEncoder
- The implementation follows Log4j2's ByteBufferDestination interface contract
- Buffer access is limited to Log4j2's internal encoding process
- The code includes proper buffer management:
- Buffer is grown systematically when needed
- Clear documentation indicates buffer must be cleared before reuse
- No sensitive data handling is involved - it's purely for log message formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check buffer exposure implementation
# Search for buffer exposure in PatternLayoutBufferDestination
ast-grep --pattern 'class PatternLayoutBufferDestination {
$$$
public $_ getBuffer() {
$$$
}
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Search for PatternLayoutBufferDestination class implementation
rg -l "PatternLayoutBufferDestination"
# Then examine the buffer handling
rg "getBuffer|buffer" -A 5 -B 5
Length of output: 16139
44-48: Review custom synchronization implementation
The synchronization exclusion requires careful review as custom thread synchronization can be error-prone.
✅ Verification successful
Custom synchronization implementation is properly designed and justified
The synchronization implementation in AbstractBufferedRollingFileAppender is well-structured and follows good practices:
- Uses volatile fields appropriately for thread visibility
- Implements synchronized methods for thread-safe operations
- Has clear documentation explaining synchronization decisions
- Uses atomic operations where appropriate (e.g.,
AtomicBoolean,AtomicLong) - Avoids synchronization in specific cases with documented reasons (e.g.,
stop()method) - Handles concurrent access from multiple logging threads and background threads
The SpotBugs exclusion is justified as the custom synchronization is specifically designed for managing shutdown sequences after user threads complete, with proper volatile fields and synchronized blocks in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Examine synchronization implementation
# Search for synchronized blocks and volatile fields
ast-grep --pattern 'class AbstractBufferedRollingFileAppender {
$$$
private volatile $_ $_;
$$$
synchronized $_ $_($_) {
$$$
}
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Let's try a different approach to examine the synchronization implementation
# First, let's find the actual file containing the class
fd "AbstractBufferedRollingFileAppender.java"
# Then examine the synchronization patterns
rg -A 5 -B 5 "synchronized|volatile" --type java
Length of output: 18493
2-18: Verify constructor throw exclusions
The exclusions for constructor throws are reasonable for Log4j2 appenders as they should fail fast when misconfigured. However, we should ensure proper error messages are provided to users.
✅ Verification successful
Constructor throw exclusions are properly justified
The SpotBugs exclusions for constructor throws are appropriate. The code shows that:
- ClpIrFileAppender throws IllegalArgumentException with clear messages for invalid compression levels and missing file configurations
- CompressionPatternLayoutContainer performs null checks using Objects.requireNonNull
- All constructors have proper parameter validation with descriptive error messages
The "fail-fast" approach implemented here is a good practice for configuration validation in Log4j2 appenders, making troubleshooting easier for users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that constructors throw with descriptive messages
# Search for constructor implementations to verify error handling
ast-grep --pattern 'class $_ {
$$$
public $_($_) throws $_ {
$$$
throw new $_($_);
$$$
}
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Let's try a different approach to find constructor implementations
# Search for specific class files mentioned in the SpotBugs exclusions
# Search for AbstractBufferedRollingFileAppender
rg -l "class AbstractBufferedRollingFileAppender" | xargs rg -A 10 "AbstractBufferedRollingFileAppender\("
# Search for ClpIrFileAppender
rg -l "class ClpIrFileAppender" | xargs rg -A 10 "ClpIrFileAppender\("
# Search for CompressionPatternLayoutContainer
rg -l "class CompressionPatternLayoutContainer" | xargs rg -A 10 "CompressionPatternLayoutContainer\("
# Also search for IllegalArgumentException or ConfigurationException throws
rg "throw new (IllegalArgumentException|ConfigurationException)" -A 2
Length of output: 3228
26-42: Validate Log4j2 plugin attribute usage
The unread field exclusions for builder classes are justified as Log4j2 uses reflection for plugin attributes. However, we should verify that these fields are properly annotated with @PluginAttribute.
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax. Let's also try with a simpler pattern:
SpotBugs exclusions for unread fields are correctly justified
The code search confirms that all Builder classes mentioned in the SpotBugs exclusions properly use @PluginBuilderAttribute annotations on their fields:
ClpIrFileAppender.Builder: Uses annotations forcloseFrameOnFlush,useFourByteEncoding,fileNameAbstractBufferedRollingFileAppender.Builder: Uses annotations forbaseName,closeOnShutdown,flushHardTimeoutsInMinutes,flushSoftTimeoutsInSecondsAbstractClpIrBufferedRollingFileAppender.Builder: Uses annotations forcompressionLevel,rolloverCompressedSizeThreshold,rolloverUncompressedSizeThreshold
These fields are indeed used by Log4j2's plugin system through reflection, justifying their SpotBugs exclusions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify plugin attribute annotations
# Search for builder classes and their field declarations
ast-grep --pattern 'class $_Builder {
$$$
@PluginAttribute($_)
private $_ $_;
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Let's try a different approach to find the builder classes and their fields
# First, let's find the relevant Java files
fd -e java . | xargs rg -l "Builder.*class"
# Then let's examine these files for plugin attributes
fd -e java . | xargs rg "@PluginAttribute" -B 2 -A 2
Length of output: 98
Script:
#!/bin/bash
ast-grep --pattern 'class $_ {
@PluginAttribute
$_
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try searching for the specific class names mentioned in the spotbugs-exclude.xml
rg -l "AbstractBufferedRollingFileAppender|AbstractClpIrBufferedRollingFileAppender|ClpIrFileAppender" --type java
# Once we find the files, let's look at their content
fd -e java . | xargs rg "class.*Builder" -A 10
Length of output: 4425
src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java (1)
1-324: Implementation looks solid!
The implementation is well-structured, properly documented, and includes comprehensive parameter validation. The class effectively extends AbstractBufferedRollingFileAppender to provide CLP compression functionality with configurable rollover thresholds.
src/main/java/com/yscope/logging/log4j2/ClpIrFileAppender.java (1)
305-338: LGTM! Well-implemented Builder pattern
The Builder implementation follows best practices:
- Proper fluent interface with method chaining
- Good parameter validation in
validateParameters() - Clear default values for boolean flags
| */ | ||
| public class ClpIrFileAppender extends AbstractAppender implements Flushable { | ||
| public static final String PLUGIN_NAME = "ClpIrFileAppender"; | ||
| private static final int estimatedFormattedTimestampLength = 0; |
There was a problem hiding this comment.
Fix incorrect initialization of estimatedFormattedTimestampLength
The static field estimatedFormattedTimestampLength is initialized to 0, which seems incorrect as it's used in append() method to calculate uncompressedSizeInBytes. This could lead to inaccurate size calculations.
Consider calculating a reasonable estimate based on the timestamp pattern format, for example:
- private static final int estimatedFormattedTimestampLength = 0;
+ private static final int estimatedFormattedTimestampLength = 24; // e.g., "2024-11-20T12:34:56.789-0500"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final int estimatedFormattedTimestampLength = 0; | |
| private static final int estimatedFormattedTimestampLength = 24; // e.g., "2024-11-20T12:34:56.789-0500" |
There was a problem hiding this comment.
@p1halani Is this accurate?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
It should be 25. Updated
| public void append(final LogEvent event) { | ||
| // We synchronize on the lock to ensure that the appender is not stopped while | ||
| // we are appending and can be called by multiple threads | ||
| synchronized (lock) { | ||
| // No-OP if we do not need to do any work | ||
| if (false == isStarted() || isStopping() || isStopped()) { return; } | ||
|
|
||
| event.getLevel(); | ||
|
|
||
| try { | ||
| ByteBuffer logMsg = compressionLayoutContainer.encodeLogMsg(event); | ||
| Objects.requireNonNull(clpIrOutputStream).writeLogEvent( | ||
| event.getTimeMillis(), | ||
| logMsg | ||
| ); | ||
| uncompressedSizeInBytes += estimatedFormattedTimestampLength + logMsg.limit(); | ||
| } catch (Exception ex) { | ||
| error("Failed to append log event for appender " + getName(), event, ex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary call and optimize synchronization
Two issues in the append method need attention:
- Line 175 calls
event.getLevel()but doesn't use the result - The entire method body is synchronized which could impact performance
Consider these improvements:
public void append(final LogEvent event) {
synchronized (lock) {
if (false == isStarted() || isStopping() || isStopped()) { return; }
- event.getLevel();
try {
ByteBuffer logMsg = compressionLayoutContainer.encodeLogMsg(event);
+ // Minimize synchronized block to only protect stream operations
+ synchronized (clpIrOutputStream) {
Objects.requireNonNull(clpIrOutputStream).writeLogEvent(
event.getTimeMillis(),
logMsg
);
uncompressedSizeInBytes += estimatedFormattedTimestampLength + logMsg.limit();
+ }
} catch (Exception ex) {
error("Failed to append log event for appender " + getName(), event, ex);
}
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@p1halani I think comment about line 175 is accurate?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| public ClpIrFileAppender( | ||
| final String filePath, | ||
| final String name, | ||
| boolean ignoreExceptions, | ||
| final PatternLayout layout, | ||
| final Filter filter, | ||
| int compressionLevel, | ||
| boolean useFourByteEncoding, | ||
| boolean closeFrameOnFlush | ||
| ) | ||
| throws IOException { | ||
| super(name, filter, layout, ignoreExceptions, Property.EMPTY_ARRAY); | ||
| this.file = filePath; | ||
| this.useFourByteEncoding = useFourByteEncoding; | ||
| this.compressionLevel = compressionLevel; | ||
| this.closeFrameOnFlush = closeFrameOnFlush; | ||
|
|
||
| compressionLayoutContainer = new CompressionPatternLayoutContainer(layout); | ||
| startHelper(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve constructor safety and validation
The constructor has two potential issues:
- It performs I/O operations directly by calling
startHelper(false), which could throw exceptions during object construction - Input parameters aren't validated before use
Consider separating initialization from construction:
public ClpIrFileAppender(...) throws IOException {
super(name, filter, layout, ignoreExceptions, Property.EMPTY_ARRAY);
+ if (layout == null) {
+ throw new IllegalArgumentException("Layout cannot be null");
+ }
+ if (filePath == null || filePath.trim().isEmpty()) {
+ throw new IllegalArgumentException("File path cannot be null or empty");
+ }
this.file = filePath;
this.useFourByteEncoding = useFourByteEncoding;
this.compressionLevel = compressionLevel;
this.closeFrameOnFlush = closeFrameOnFlush;
compressionLayoutContainer = new CompressionPatternLayoutContainer(layout);
- startHelper(false);
}
+ /**
+ * Initializes the appender after construction.
+ * @throws IOException if initialization fails
+ */
+ public void initialize() throws IOException {
+ startHelper(false);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ClpIrFileAppender( | |
| final String filePath, | |
| final String name, | |
| boolean ignoreExceptions, | |
| final PatternLayout layout, | |
| final Filter filter, | |
| int compressionLevel, | |
| boolean useFourByteEncoding, | |
| boolean closeFrameOnFlush | |
| ) | |
| throws IOException { | |
| super(name, filter, layout, ignoreExceptions, Property.EMPTY_ARRAY); | |
| this.file = filePath; | |
| this.useFourByteEncoding = useFourByteEncoding; | |
| this.compressionLevel = compressionLevel; | |
| this.closeFrameOnFlush = closeFrameOnFlush; | |
| compressionLayoutContainer = new CompressionPatternLayoutContainer(layout); | |
| startHelper(false); | |
| } | |
| public ClpIrFileAppender( | |
| final String filePath, | |
| final String name, | |
| boolean ignoreExceptions, | |
| final PatternLayout layout, | |
| final Filter filter, | |
| int compressionLevel, | |
| boolean useFourByteEncoding, | |
| boolean closeFrameOnFlush | |
| ) | |
| throws IOException { | |
| super(name, filter, layout, ignoreExceptions, Property.EMPTY_ARRAY); | |
| if (layout == null) { | |
| throw new IllegalArgumentException("Layout cannot be null"); | |
| } | |
| if (filePath == null || filePath.trim().isEmpty()) { | |
| throw new IllegalArgumentException("File path cannot be null or empty"); | |
| } | |
| this.file = filePath; | |
| this.useFourByteEncoding = useFourByteEncoding; | |
| this.compressionLevel = compressionLevel; | |
| this.closeFrameOnFlush = closeFrameOnFlush; | |
| compressionLayoutContainer = new CompressionPatternLayoutContainer(layout); | |
| } | |
| /** | |
| * Initializes the appender after construction. | |
| * @throws IOException if initialization fails | |
| */ | |
| public void initialize() throws IOException { | |
| startHelper(false); | |
| } |
kirkrodrigues
changed the title
Description
This PR:
It uses
clp-ffiandzstd-jnifor compression.Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Boilerplate.javaas it was no longer needed.