The type converters still seems like a great idea. I've never liked having all of this boilerplate conversion code all over that place, even if most of it was refactored into calls to utils. It's too easy to make a mistake.
WRT default values, then yes, we need to carefully consider what to do here, but since we are the primary customer, we can do whatever we want especially for the tests. Finger waving? ;-) sometimes a big pile of changes is safe, sometimes one character change is not... :-( What happened to running Log4J on a CI server? This would not solve this process issue but it would feedback an error to the ML and the committer ASAP. Gary On Sun, May 25, 2014 at 8:11 AM, Remko Popma <[email protected]> wrote: > I'm seeing a compile error in FlumeAppender (after the change > to Rfc5424Layout.createLayout). I fixed that, but now there are > few broken JUnit tests for the Flume appender: > > Tests in error: > FlumeAppenderTest.testBatch:237 ≫ IllegalArgument No structured id name > was su... > FlumeAppenderTest.testStructured:163 ≫ IllegalArgument No structured id > name w... > FlumeAppenderTest.testLog4jAvroAppender:134 ≫ IllegalArgument No > structured id... > FlumeAppenderTest.testNotConnected:301 ≫ IllegalArgument No structured > id name... > FlumeAppenderTest.testMultiple:203 ≫ IllegalArgument No structured id > name was... > FlumeAppenderTest.testReconnect:349 ≫ IllegalArgument No structured id > name wa... > FlumeAppenderTest.testConnectionRefused:271 ≫ IllegalArgument No > structured id... > > detail: > > testBatch(org.apache.logging.log4j.flume.appender.FlumeAppenderTest) Time > elapsed: 3.305 sec <<< ERROR! > java.lang.IllegalArgumentException: No structured id name was supplied > at > org.apache.logging.log4j.message.StructuredDataId.<init>(StructuredDataId.java:92) > at > org.apache.logging.log4j.core.layout.Rfc5424Layout.<init>(Rfc5424Layout.java:136) > at > org.apache.logging.log4j.core.layout.Rfc5424Layout.createLayout(Rfc5424Layout.java:652) > at > org.apache.logging.log4j.flume.appender.FlumeAppender.createAppender(FlumeAppender.java:223) > at > org.apache.logging.log4j.flume.appender.FlumeAppenderTest.testBatch(FlumeAppenderTest.java:237) > > > First, the mandatory finger wagging: please please run the tests before > committing, etc, etc. > > Second, I'm looking at how to fix these tests and now I'm not sure whether > the default annotation was a good idea: the thing is that the JUnit tests > use the factory methods directly but there is no PluginManager involved, so > nobody supplies the default value any more... > > That is why the JUnit tests above fail: FlumeAppender.createAppender() > passes a null argument to Rfc5424Layout.createLayout(), which is no longer > converted to the default value, which causes issues downstream. > > This raises the question: how do we deal with this? > > Should we change the FlumeAppender.createAppender() to provide a non-null > value? > That is possible of course, but then we only test the cases where a value > was supplied. Would we not be missing test cases this way? > > UPDATE: > I've tried providing a non-null value but the tests still fail. > > I want to work on other things now so I'm giving up on this. Matt, could > you take a look? > > Error detail: > testBatch(org.apache.logging.log4j.flume.appender.FlumeAppenderTest) Time > elapsed: 3.315 sec <<< ERROR! > org.apache.logging.log4j.core.appender.AppenderLoggingException: An > exception occurred processing Appender avro > at > org.apache.logging.log4j.core.net.Priority.getPriority(Priority.java:46) > at > org.apache.logging.log4j.core.layout.Rfc5424Layout.appendPriority(Rfc5424Layout.java:283) > at > org.apache.logging.log4j.core.layout.Rfc5424Layout.toSerializable(Rfc5424Layout.java:262) > at > org.apache.logging.log4j.core.layout.Rfc5424Layout.toSerializable(Rfc5424Layout.java:63) > at > org.apache.logging.log4j.core.layout.AbstractStringLayout.toByteArray(AbstractStringLayout.java:46) > at > org.apache.logging.log4j.flume.appender.FlumeAppender.append(FlumeAppender.java:102) > at > org.apache.logging.log4j.core.config.AppenderControl.callAppender(AppenderControl.java:97) > at > org.apache.logging.log4j.core.config.LoggerConfig.callAppenders(LoggerConfig.java:426) > at > org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:405) > at > org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:365) > at org.apache.logging.log4j.core.Logger.logMessage(Logger.java:103) > at > org.apache.logging.log4j.spi.AbstractLoggerProvider.logMessage(AbstractLoggerProvider.java:1336) > at > org.apache.logging.log4j.spi.AbstractLoggerProvider.logIfEnabled(AbstractLoggerProvider.java:1325) > at org.apache.logging.log4j.spi.AbstractLoggerProvider.info > (AbstractLoggerProvider.java:907) > at > org.apache.logging.log4j.flume.appender.FlumeAppenderTest.testBatch(FlumeAppenderTest.java:248) > -- E-Mail: [email protected] | [email protected] Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> Spring Batch in Action <http://www.manning.com/templier/> Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
