You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/11/19 11:33:31 UTC
[GitHub] [tomcat] martin-g commented on a change in pull request #378: use a single `Thread` to do `async-delete-file` works instead of `ThreadPoolExecutor`
martin-g commented on a change in pull request #378:
URL: https://github.com/apache/tomcat/pull/378#discussion_r526794048
##########
File path: java/org/apache/juli/FileHandler.java
##########
@@ -18,37 +18,20 @@
package org.apache.juli;
-import java.io.BufferedOutputStream;
-import java.io.File;
-import java.io.FileOutputStream;
-import java.io.IOException;
-import java.io.OutputStream;
-import java.io.OutputStreamWriter;
-import java.io.PrintWriter;
-import java.io.UnsupportedEncodingException;
+import java.io.*;
Review comment:
Please don't use wildcard imports
##########
File path: java/org/apache/juli/FileHandler.java
##########
@@ -58,98 +41,48 @@
* <p>The following configuration properties are available:</p>
*
* <ul>
- * <li><code>directory</code> - The directory where to create the log file.
- * If the path is not absolute, it is relative to the current working
- * directory of the application. The Apache Tomcat configuration files usually
- * specify an absolute path for this property,
- * <code>${catalina.base}/logs</code>
- * Default value: <code>logs</code></li>
- * <li><code>rotatable</code> - If <code>true</code>, the log file will be
- * rotated on the first write past midnight and the filename will be
- * <code>{prefix}{date}{suffix}</code>, where date is yyyy-MM-dd. If <code>false</code>,
- * the file will not be rotated and the filename will be <code>{prefix}{suffix}</code>.
- * Default value: <code>true</code></li>
- * <li><code>prefix</code> - The leading part of the log file name.
- * Default value: <code>juli.</code></li>
- * <li><code>suffix</code> - The trailing part of the log file name. Default value: <code>.log</code></li>
- * <li><code>bufferSize</code> - Configures buffering. The value of <code>0</code>
- * uses system default buffering (typically an 8K buffer will be used). A
- * value of <code><0</code> forces a writer flush upon each log write. A
- * value <code>>0</code> uses a BufferedOutputStream with the defined
- * value but note that the system default buffering will also be
- * applied. Default value: <code>-1</code></li>
- * <li><code>encoding</code> - Character set used by the log file. Default value:
- * empty string, which means to use the system default character set.</li>
- * <li><code>level</code> - The level threshold for this Handler. See the
- * <code>java.util.logging.Level</code> class for the possible levels.
- * Default value: <code>ALL</code></li>
- * <li><code>filter</code> - The <code>java.util.logging.Filter</code>
- * implementation class name for this Handler. Default value: unset</li>
- * <li><code>formatter</code> - The <code>java.util.logging.Formatter</code>
- * implementation class name for this Handler. Default value:
- * <code>java.util.logging.SimpleFormatter</code></li>
- * <li><code>maxDays</code> - The maximum number of days to keep the log
- * files. If the specified value is <code><=0</code> then the log files
- * will be kept on the file system forever, otherwise they will be kept the
- * specified maximum days. Default value: <code>-1</code>.</li>
+ * <li><code>directory</code> - The directory where to create the log file.
+ * If the path is not absolute, it is relative to the current working
+ * directory of the application. The Apache Tomcat configuration files usually
+ * specify an absolute path for this property,
+ * <code>${catalina.base}/logs</code>
+ * Default value: <code>logs</code></li>
+ * <li><code>rotatable</code> - If <code>true</code>, the log file will be
+ * rotated on the first write past midnight and the filename will be
+ * <code>{prefix}{date}{suffix}</code>, where date is yyyy-MM-dd. If <code>false</code>,
+ * the file will not be rotated and the filename will be <code>{prefix}{suffix}</code>.
+ * Default value: <code>true</code></li>
+ * <li><code>prefix</code> - The leading part of the log file name.
+ * Default value: <code>juli.</code></li>
+ * <li><code>suffix</code> - The trailing part of the log file name. Default value: <code>.log</code></li>
+ * <li><code>bufferSize</code> - Configures buffering. The value of <code>0</code>
+ * uses system default buffering (typically an 8K buffer will be used). A
+ * value of <code><0</code> forces a writer flush upon each log write. A
+ * value <code>>0</code> uses a BufferedOutputStream with the defined
+ * value but note that the system default buffering will also be
+ * applied. Default value: <code>-1</code></li>
+ * <li><code>encoding</code> - Character set used by the log file. Default value:
+ * empty string, which means to use the system default character set.</li>
+ * <li><code>level</code> - The level threshold for this Handler. See the
+ * <code>java.util.logging.Level</code> class for the possible levels.
+ * Default value: <code>ALL</code></li>
+ * <li><code>filter</code> - The <code>java.util.logging.Filter</code>
+ * implementation class name for this Handler. Default value: unset</li>
+ * <li><code>formatter</code> - The <code>java.util.logging.Formatter</code>
+ * implementation class name for this Handler. Default value:
+ * <code>java.util.logging.SimpleFormatter</code></li>
+ * <li><code>maxDays</code> - The maximum number of days to keep the log
+ * files. If the specified value is <code><=0</code> then the log files
+ * will be kept on the file system forever, otherwise they will be kept the
+ * specified maximum days. Default value: <code>-1</code>.</li>
Review comment:
Whitespace changes which are not really needed.
##########
File path: java/org/apache/juli/FileHandler.java
##########
@@ -585,4 +517,34 @@ private String obtainDateFromPath(Path path) {
return null;
}
}
+
+
+ static class AsyncTaskExecutor {
+
+ private Thread thread;
+ private BlockingQueue<Runnable> tasks;
+
+ AsyncTaskExecutor() {
+ this.tasks = new LinkedBlockingQueue<>();
+ this.thread = new Thread(() -> {
+ try {
+ this.tasks.take().run();
+ } catch (Exception e) {
+ System.err.println("take task failed");
+ e.printStackTrace();
+ }
+ });
+ this.thread.setName("tomcat-async-delete-file");
+ this.thread.start();
Review comment:
The thread should be set as a daemon, or the class should be a lifecycle listener and stop the thread when Tomcat shuts down.
##########
File path: java/org/apache/juli/FileHandler.java
##########
@@ -585,4 +517,34 @@ private String obtainDateFromPath(Path path) {
return null;
}
}
+
+
+ static class AsyncTaskExecutor {
+
+ private Thread thread;
+ private BlockingQueue<Runnable> tasks;
+
+ AsyncTaskExecutor() {
+ this.tasks = new LinkedBlockingQueue<>();
+ this.thread = new Thread(() -> {
+ try {
+ this.tasks.take().run();
+ } catch (Exception e) {
+ System.err.println("take task failed");
+ e.printStackTrace();
Review comment:
Please use a Logger
##########
File path: java/org/apache/juli/FileHandler.java
##########
@@ -585,4 +517,34 @@ private String obtainDateFromPath(Path path) {
return null;
}
}
+
+
+ static class AsyncTaskExecutor {
+
+ private Thread thread;
+ private BlockingQueue<Runnable> tasks;
+
+ AsyncTaskExecutor() {
+ this.tasks = new LinkedBlockingQueue<>();
+ this.thread = new Thread(() -> {
+ try {
Review comment:
This should probably be wrapped in a `while (true)` loop. At the moment it will execute just the first task and finish.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org