It could easily be that Matt ran all the tests and they all passed, but he just forgot to commit the FlumeAppender file. That would explain the compilation error.
I'm not against the type conversion, please don't misunderstand. About the @default annotations I'm still forming an opinion. I can see the benefit, but something bothers me about the JUnit tests, still trying to figure out what it is exactly that bothers me... When users run this code they will go through the PluginManager, and the @default annotations will guarantee certain values. In our previous mechanism to provide defaults, the JUnit tests could use those defaults by specifying null. With the new mechanism JUnit tests cannot use defaults. Tests need to specify a value. So now it becomes easy to make a mistake in the JUnit test. (The factory methods all have tons of parameters so they are already easy to get wrong in the test...) Also, we need to keep the JUnit test in sync when default values change (before a JUnit test specifying null would automatically pick up the new default). On Sun, May 25, 2014 at 10:24 PM, Gary Gregory <[email protected]>wrote: > 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 >
