Skip to content

Conversation

ppkarwasz
Copy link
Contributor

We move the DataSource connection source to a new module log4j-jdbc-jndi.

The main rationale for this change is to remove the optional dependency on log4j-jndi from log4j-jdbc.
This change forces users to opt-in for the JNDI DataSource by adding log4j-jdbc-jndi to their classpath.

After this change, the log4j-jdbc artifact contains:

  • the JDBC Appender,
  • a connection source based on DriverManager,
  • a connection source based on a static factory method.

Closes #1914

@ppkarwasz ppkarwasz added this to the 3.x milestone Sep 2, 2024
@ppkarwasz ppkarwasz self-assigned this Sep 3, 2024
@ppkarwasz ppkarwasz requested a review from vy September 3, 2024 10:47
@vy vy added the appenders:JDBC Issue concerning the JDBC appender label Sep 4, 2024
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM.

Question: Is the DBCP detail of our log4j-jdbc* modules well-hidden? If so, I suggest

  1. Removing all of its public mentions
  2. Switching to HikariCP1 instead

1 HikariCP is, both performance- and feature-wise, a clear winner against any other alternative in the market as of today. It is the default JDBC connection pool in both Spring Boot 2 and Spring Boot 3.

Of course I am not suggesting this for this PR. But I feel like this is an okay place to discuss it.

We move the [`DataSource` connection source](https://logging.staged.apache.org/log4j/3.x/manual/appenders/database.html#DataSourceConnectionSource)
to a new module `log4j-jdbc-jndi`.

The main rationale for this change is to remove the **optional** dependency
on `log4j-jndi` from `log4j-jdbc`.
This change forces users to opt-in for the JNDI `DataSource` by adding `log4j-jdbc-jndi` to their classpath.

After this change, the `log4j-jdbc` artifact contains:

* the `JDBC` Appender,
* a connection source based on `DriverManager`,
* a connection source based on a static factory method.

Closes #1914
@ppkarwasz ppkarwasz force-pushed the feature/main/1914_log4j_jdbc_jndi branch from af3b21b to 5a5224e Compare September 6, 2024 08:05
@ppkarwasz ppkarwasz merged commit 5a5224e into main Sep 6, 2024
3 of 8 checks passed
@ppkarwasz ppkarwasz deleted the feature/main/1914_log4j_jdbc_jndi branch September 6, 2024 08:06
@ppkarwasz
Copy link
Contributor Author

Question: Is the DBCP detail of our log4j-jdbc* modules well-hidden? If so, I suggest

1. Removing all of its public mentions

2. Switching to HikariCP instead

DBCP has a nice feature, that justifies its pool of aficionados: you can register a pool with the JDBC driver and retrieve it later using a jdbc:apache:commons URI.

Note that until #1916 is fixed, all pools perform the same: only a single connection is used at each time.
The main problem is the synchronization of AbstractDatabaseManager. Removing it might technically not be a breaking change, but it is a very disruptive one.

ppkarwasz added a commit that referenced this pull request Sep 12, 2024
We move the [`DataSource` connection source](https://logging.staged.apache.org/log4j/3.x/manual/appenders/database.html#DataSourceConnectionSource)
to a new module `log4j-jdbc-jndi`.

The main rationale for this change is to remove the **optional** dependency
on `log4j-jndi` from `log4j-jdbc`.
This change forces users to opt-in for the JNDI `DataSource` by adding `log4j-jdbc-jndi` to their classpath.

After this change, the `log4j-jdbc` artifact contains:

* the `JDBC` Appender,
* a connection source based on `DriverManager`,
* a connection source based on a static factory method.

Closes #1914
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

appenders:JDBC Issue concerning the JDBC appender

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants