[GitHub] [tomcat] markt-asf commented on a diff in pull request #583: Replaced old time API

["markt-asf (via GitHub)" <gi...@apache.org>]

markt-asf commented on code in PR #583:
URL: https://github.com/apache/tomcat/pull/583#discussion_r1105766663


##########
java/org/apache/catalina/filters/RequestDumperFilter.java:
##########
@@ -56,12 +59,13 @@ public class RequestDumperFilter extends GenericFilter {
     private static final String NON_HTTP_REQ_MSG = "Not available. Non-http request.";
     private static final String NON_HTTP_RES_MSG = "Not available. Non-http response.";
 
-    private static final ThreadLocal<Timestamp> timestamp = ThreadLocal.withInitial(Timestamp::new);
-
     // Log must be non-static as loggers are created per class-loader and this
     // Filter may be used in multiple class loaders
     private transient Log log = LogFactory.getLog(RequestDumperFilter.class);
 
+    private static final ZoneId ZONE_ID = ZoneId.systemDefault();
+    private static final DateTimeFormatter DF_DATETIME = DateTimeFormatter.ofPattern("dd-MM-yyyy HH:mm:ss");
+    private static final AtomicLong ATOMIC_LONG = new AtomicLong();

Review Comment:
   Need better names - at least for the AtomicLong



##########
java/org/apache/catalina/filters/RequestDumperFilter.java:
##########
@@ -239,17 +243,11 @@ private void doLog(String attribute, String value) {
     }
 
     private String getTimestamp() {
-        Timestamp ts = timestamp.get();
         long currentTime = System.currentTimeMillis();
-
-        if ((ts.date.getTime() + 999) < currentTime) {
-            ts.date.setTime(currentTime - (currentTime % 1000));
-            ts.update();
-        }
-        return ts.dateString;
+        long res = ATOMIC_LONG.updateAndGet(it -> it + 999 < currentTime ? currentTime - (currentTime % 1000) : it);

Review Comment:
   This appears to create a source of contention across threads - exactly what the current code is designed to avoid.



##########
java/org/apache/catalina/manager/JspHelper.java:
##########
@@ -72,14 +74,19 @@ public static String guessDisplayUserFromSession(Session in_session) {
         return escapeXml(user);
     }
 
+    private static String getDateTimeStr(long value) {
+        Instant instant = Instant.ofEpochMilli(value);
+        LocalDateTime ldt = LocalDateTime.ofInstant(instant, ZONE_ID);
+        return DATE_TIME_FORMAT.format(ldt);
+    }
 

Review Comment:
   It is different. But why is it better?



##########
java/org/apache/catalina/util/Strftime.java:
##########
@@ -113,36 +113,52 @@ public class Strftime {
      */
     public Strftime( String origFormat, Locale locale ) {
         String convertedFormat = convertDateFormat( origFormat );
-        simpleDateFormat = new SimpleDateFormat( convertedFormat, locale );
+        dtf = DateTimeFormatter.ofPattern(convertedFormat).withLocale(locale);
     }
 
     /**
      * Format the date according to the strftime-style string given in the constructor.
      *
-     * @param date the date to format
+     * @param instant the date to format
      * @return the formatted date
      */
-    public String format( Date date ) {
-        return simpleDateFormat.format( date );
+    public String format(Instant instant) {

Review Comment:
   Changes to public APIs need more work. Generally, the old API is deprecated in the latest stable release (and possible older releases) and the new API is added to at least the current development release. The new API may be back-ported to older releases where it makes sense to do so.



##########
java/org/apache/tomcat/dbcp/pool2/impl/DefaultPooledObjectInfo.java:
##########
@@ -30,7 +33,7 @@
  */
 public class DefaultPooledObjectInfo implements DefaultPooledObjectInfoMBean {
 
-    private static final String PATTERN = "yyyy-MM-dd HH:mm:ss Z";
+    private static final DateTimeFormatter DTF = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss Z");
 
     private final PooledObject<?> pooledObject;
 

Review Comment:
   This code is forked from Commons Pool. Changes need to be made there.



-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

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