Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log compression #223

Closed
wants to merge 15 commits into from
Closed

Log compression #223

wants to merge 15 commits into from

Conversation

habuzahra
Copy link

@habuzahra habuzahra commented Nov 14, 2019

This modification has been done based on the below bugzilla

https://bz.apache.org/bugzilla/show_bug.cgi?id=62611

Below are the modifications that has been done during the task:

1- org.apache.juli.FileHandler.compress();
2- org.apache.juli.FileHandler.deleteRotatedFile();
3- org.apache.catalina.valves compress();
4- org.apache.catalina.valves.deleteRotatedFile();
5- External configurations, where you can enable or disable the compression function based on new configurations keys to located in logging.properties
1catalina.org.apache.juli.AsyncFileHandler.compress = true
2localhost.org.apache.juli.AsyncFileHandler.compress = true
3manager.org.apache.juli.AsyncFileHandler.compress = true
4host-manager.org.apache.juli.AsyncFileHandler.compress = true

/**
* Determines whether the log file should be compressed
*/
private Boolean compress;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be documented in the class level Javadoc

@@ -393,6 +430,9 @@ private void configure() {
if (suffix == null) {
suffix = getProperty(className + ".suffix", ".log");
}
if (compress == null) {
compress = Boolean.valueOf(getProperty(className + ".compress", "true"));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like some tabs have crept in here. Tomcat uses 4 spaces for idents. I recommend enabling Checkstyle for you build as it will catch this sort of thing,

@@ -0,0 +1,50 @@
package org.apache.tomcat.util.file;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs an ALv2 header. Again Checkstyle will catch this.

@@ -37,6 +37,7 @@
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.buf.B2CConverter;
import org.apache.tomcat.util.file.CompressFileUtils;

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the JULI version since the dependency is already there rather than duplicating the code.

@markt-asf markt-asf closed this May 21, 2021
@markt-asf markt-asf deleted the branch apache:master May 21, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants