On Nov 9, 2004, at 10:35 AM, Ceki G�lc� wrote:
Hello Curt,
At first I had trouble understanding some of the constructions in CachedDateFormat but I am starting to get the hang of it. I particularly like the idea of being able to dynamically detect the placement of the millisecond field and adapt the cache accordingly. It's pretty good stuff.
Previously you mentioned that using CachedDateFormat to wrap the SimpleDateFormat created in DatePatternConverter did drop times from about 29 ms to 22 ms. What do these numbers represent? What do they measure *exactly*?
I was running the standard performance benchmarks with a null adapter by running:
ant -f performance.xml null
Before the changes, the time per log for the "{yyyy-MM-dd HH:mm:ss,SSS}" pattern was reported as 29 ms, afterwards as 22 ms on a 2.6 GHz P4 running JDK 1.4.2_03-b02 for x86 Linux. I didn't investigate what that benchmark was actually testing, but using a null adapter seemed like to most appropriate to test. If writing to a slow network or file system, the change in the layout speed might be insignificant.
Looking at CachedDateFormat I also noticed that it attempts to handle the digit zero in a locale dependent manner. However, the code of CachedDateFormat which you supplied with bug #32064 [0] does not seem to be able deal with a user specified locale, that is, a locale different than the default locale.
I believe that caching does work with non-default locales, just not ones that use non-arabic digit sets (see following paragraph). CachedDateFormatTestCase.test5 tests caching of a Thai locale date format. The test can't detect if caching actually occurs, only if it was done incorrectly, but I'm pretty sure that I stepped through it and things seemed to be working okay.
Looks like I got lazy with the implementation of findMillisecondStart. It would be better to modify the code like:
String plus987... +char nineDigit = numericFormatter.format(987).charAt(0); ... if (formattedCharAt(i) == zeroDigit && plus987.charAt(i) == nineDigit)
that should hopefully handle most if not all alternative digit sets. I had assumed incorrectly that the Thai locale would use Thai digits, but was wrong. I don't know if any of the standard locales use non-arabic digit sets.
I've also noticed that you modified PatternLayout to set the timezone and locale for all of the converters each time something needs to be formatted which is obviously a total overkill. (I guess you were just testing the code.) As I suggested earlier, I think we can get away by just allowing the user to set the timezone and locale as an option within the %d{} conversion word, for example %d{yyyy-MM-dd HH:mm:ss,SSS z, Japan/Tokyo, jp_JP}. There is no need to add timezone and locale options to PatternConverter.
The locale setting code is in activateOption which I think is only called at the end of configuration, not for every conversion.
Instead of augmenting the pattern to incorporate time zones and locales, I added them as attributes of the pattern layout. The configuration file tests/input/patternLayout15.properties and the PatternLayoutTest.test15 demonstrate specifying locale and timezone for a layout so that one log event is appended in the default local and timezone and two specified locals and timezones.
I think the case for having Locale on the appender is pretty strong. If there were multiple locale-sensitive fields in a layout, you would expect that they should share a common locale instead of being independently specified, The case for a common TimeZone for a layout is not as strong, but it was a lot simpler to implement it that way than to modify the pattern parser.
I did not attempt to localize any of the other pattern converters by overriding setLocale. Though you could envision, for example, LevelPatternConverter providing a localized string instead of "INFO", "WARN" etc. However, I think the significant use case would be to specify a generic (aka English) locale, so that the developer who might be interpreting the log file would not have to understand the language presented to the user.
Locales other than those using the latin alphabet make it hard (at least for me) to test the output. I was not able to test the output for any such locale, e.g. Japanese, Chinese, etc... Are you able to set your machine to use these languages?
CachedDateFormatTest.test5 (at least in my private copy) uses SimpleDateFormat with a Thai locale. When stepping through the code, it appeared to work correctly with non-latin languages. I haven't tried the code on a machine with a non-latin default locale.
If you modify the specified Locale in patternLayout15.properties from "fr-FR" to "th", the corresponding file (output/patternLayout15.fr) has the month rendered in what appear to be Thai digits. Note: that modification will cause the test to fail.
Did I mention that your contribution is quite impressive? Well done.
[0] http://issues.apache.org/bugzilla/show_bug.cgi?id=32064
-- Ceki G�lc�
For log4j documentation consider "The complete log4j manual" http://www.qos.ch/shop/products/eclm/
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
