Mark, On 1/21/15 10:37 AM, Mark Thomas wrote: > On 21/01/2015 15:07, schu...@apache.org wrote: >> Author: schultz >> Date: Wed Jan 21 15:07:12 2015 >> New Revision: 1653550 >> >> URL: http://svn.apache.org/r1653550 >> Log: >> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=57473 >> Add more logging to WarWatcher, specifically checking for odd states like >> non-existent files that were just listed by the filesystem, which suggests a >> permissions problem. >> Moved log strings into LocalStrings.properties >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties >> tomcat/trunk/java/org/apache/catalina/ha/deploy/WarWatcher.java >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties?rev=1653550&r1=1653549&r2=1653550&view=diff >> ============================================================================== >> --- tomcat/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties >> Wed Jan 21 15:07:12 2015 >> @@ -45,3 +45,9 @@ farmWarDeployer.stopped=Cluster FarmWarD >> farmWarDeployer.undeployEnd=Undeployment from [{0}] finished. >> farmWarDeployer.undeployLocal=Undeploy local context [{0}] >> farmWarDeployer.watchDir=Cluster deployment is watching [{0}] for changes. >> + >> +warWatcher.checking-wars=Checking WARs in {0} >> +warWatcher.listed-file-does-not-exist={0} was detected in {1} but does not >> exist. Check directory permissions on {1}? >> +warWatcher.checking-war=Checking WAR file {0} >> +warWatcher.check-war.result=WarInfo.check() returned {0} for {1} >> +warWatcher.cant-list-watchDir=Cannot list files in WatchDir "{0}": check to >> see if it is a directory and has read permissions. > > It would have been better to stick to CamelCase format for property > names as in the vast majority of these files - including elsewhere in > this one.
Okay, I can change those. >> Modified: tomcat/trunk/java/org/apache/catalina/ha/deploy/WarWatcher.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ha/deploy/WarWatcher.java?rev=1653550&r1=1653549&r2=1653550&view=diff >> ============================================================================== >> --- tomcat/trunk/java/org/apache/catalina/ha/deploy/WarWatcher.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/ha/deploy/WarWatcher.java Wed Jan >> 21 15:07:12 2015 >> @@ -23,6 +23,7 @@ import java.util.Map; >> >> import org.apache.juli.logging.Log; >> import org.apache.juli.logging.LogFactory; >> +import org.apache.tomcat.util.res.StringManager; >> >> /** >> * The <b>WarWatcher </b> watches the deployDir for changes made to the >> @@ -36,6 +37,8 @@ public class WarWatcher { >> >> /*--Static Variables----------------------------------------*/ >> private static final Log log = LogFactory.getLog(WarWatcher.class); >> + private static final StringManager sm = >> + StringManager.getManager(Constants.Package); > > Coding style is now for up 100 chars in a code line. You probably don't > need to wrap here. ACK > Note there is the option to use the class name directly in getManager(), > removing the need for the package name constant (that usually causes > issues when refactoring in IDEs). Okay. Honestly, when I added the StringManager to the class I was a little iffy about the use of Constants.Package, but that's the way it's done in FarmWarDeployer in the same package, so I decided to leave it. Should we completely remove o.a.c.ha.deploy.Constants entirely, then? It's such a small package, it wouldn't be a big change to remove that one. >> /*--Instance Variables--------------------------------------*/ >> /** >> @@ -67,20 +70,31 @@ public class WarWatcher { >> */ >> public void check() { >> if (log.isDebugEnabled()) >> - log.debug("check cluster wars at " + watchDir); >> + log.debug(sm.getString("warWatcher.checking-wars", watchDir)); >> File[] list = watchDir.listFiles(new WarFilter()); >> - if (list == null) >> + if (list == null) { >> + log.warn(sm.getString("warWatcher.cant-list-watchDir", >> + watchDir)); > > Probably no need to wrap here either. Maybe a few other places below but > I can't tell just by looking at the code - it looks to be close to 100 > in a few places. I usually don't aggressively wrap to 80 columns, but when method calls have tons of arguments in them, I tend to wrap mostly to avoid endless scrolling to see the end of the line. Looks like my default Eclipse view can see out to 102 columns without scrolling, so I'll use that at my metric in the future. >> + >> list = new File[0]; >> + } > > I was wondering about the usefulness of a debug message here saying how > many files were found. Not sure though. With trace logging enabled, you'll see the name of every WAR file processed. If you don't see any trace logs, then we found no files. I'd never object to more trace- or debug-level logging, though. Thanks, -chris
signature.asc
Description: OpenPGP digital signature
