search

cjbhaines / Log4Net.Async

Asynchronous Log4Net appenders and forwarder
http://www.nuget.org/packages/Log4Net.Async/
MIT License
121 stars 37 forks source link

BufferSize cannot be set via XML configuration #13

Closed stuartd closed 9 years ago

stuartd commented 9 years ago

The BufferSize property in AsyncForwardingAppenderBase is a nullable integer, however log4net's XML configuration code cannot do type conversion from the string value to the nullable int.

This configuration generates a debug warning and the property is not set:

 <appender name="asyncForwarder" type="Log4Net.Async.AsyncForwardingAppender,Log4Net.Async">
      <bufferSize value="2000"/>

Reference: http://stackoverflow.com/questions/31433463/unable-to-set-property-on-object-using-value-with-acceptable-conversion-type

JohnDRoach commented 9 years ago

I just ran into this as well. Have you submitted a pull request yet stuartd?

stuartd commented 9 years ago

Making the value non-nullable is potentially a breaking change for users of the library, so I was waiting to see what the owner said.

cjbhaines commented 9 years ago

Hi stuartd, thanks for looking at this. I didn't realise the XML config couldn't handle that. This sounds like a sensible change, we just need to make sure the default is definitely used if the bufferSize is omitted from the config and 0 is not used. I will try and confirm that today.

JohnDRoach commented 9 years ago

G'Day stuartd. That is correct. The pull request I submitted bumped the nuspec version up to 2 to get around that. But now that I think about it I forgot to change the assemblyinfo.cs.

G'Day cjbhaines, thanks for responding quickly. Your library is great and gave our software a massive performance increase. 1000 is a little small for our potential burst load size though. Making this change will have it used in production in our next release :)

cjbhaines commented 9 years ago

Thanks JohnDRoach. I don't think this is a breaking change because it looks like the bufferSize property didn't even work before, so perhaps we can leave it in the same major version but do a minor bump? I have run your changes through additional unit tests which I will commit once I merge your PR.

JohnDRoach commented 9 years ago

I think a major version bump is safest given a user might have extended Async...Base or the other classes that were modified. They are part of the public api.

cjbhaines commented 9 years ago

Ok that's fine then. As we are doing a major bump I might take this opportunity to delete the AsyncAdoAppender and AsyncRollingFileAppender as they have been obsolete for a while. Any objections there?

cjbhaines commented 9 years ago

Also, I just added a testimonials section to the readme. I would be very grateful if you guys could add something :-)

JohnDRoach commented 9 years ago

Deprecating AsyncAdoAppender and AsyncRollingFileAppender is ok with me. Sure, I can add something. :)

cjbhaines commented 9 years ago

Thanks a lot! I will push NuGet packages soon.