Below... On Mon, Oct 16, 2023 at 7:38 AM Piotr P. Karwasz <piotr.karw...@gmail.com> wrote: > > Hi all, > > While checking out the module descriptors of `3.x` I have some > propositions of improvements to `log4j-jdbc` that I would like to run > by you before creating the appropriate Github issues: > > 1. The `log4j-jdbc` module depends on the optional presence of > `log4j-jndi`. Maybe we should split JNDI support into a > `log4j-jdbc-jndi` artifact. This way users that do not use JNDI can be > 100% certain that no JNDI code is present. Users that require JNDI > have a single dependency to add (`log4j-jdbc-jndi`),
Fine with me but in general I like to only split out this kind of dependency to avoid optional jar dependencies in a pom.xml. I'll go with the consensus on this one. > 2. There are two ways to map event data to columns: ColumnConfig and > ColumnMapping. It is unclear to me which way is the recommended one. > Since we can perform breaking changes in 3.x, could we remove one of > these methods? I would not REMOVE one, I would merge the functionality of both into one, probably into ColumnMapping since it is at the "db" level instead of the lower "jdbc" level. Either way, merging is the way to go, at least without taking a deep dive back into it. I can tell you that I rely on the JDBC and DBCP variant module. > 3. Literal values are inserted AS-IS into the prepared statement. Java > 9 introduced `Statement#enquoteLiteral`, maybe we should use it by > default, so users are not required to quote the value themselves. We > can provide an additional `quoteLiteral` attribute with a default of > true, to allow users to add whatever they want to the prepared > statement unquoted, I can't say without trying the API and seeing how it works in practice. I would do 3 above first to avoid duplicating work. > 4. The appender itself is quite wasteful in the number of connections > it uses (one per log message). IIRC JDBC connections are not > thread-safe, but can be used from multiple threads if synchronization > is provided. `AbstractDatabaseManager#write` provides such a > synchronization. With the current connection usage pattern, the > "DriverManager" connection source is basically useless. > 5. We should consider integrating the JDBC pooling features with PR > 1401: https://github.com/apache/logging-log4j2/pull/1401. Pooling is already supported by the jdbc-dbcp2 module. What am I missing? Gary > > Piotr
