Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

including back slf4j api for apache http #1460

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

BentsiLeviav
Copy link
Contributor

The new default HTTP library (Apache HTTP) requires a slf4j dependency. Failing to provide this dependency will result in exceptions, leading the system to fall back to an alternative HTTP library.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Bentsi Leviav seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mzitnik mzitnik merged commit 2d86b94 into main Sep 28, 2023
65 checks passed
@rsim
Copy link

rsim commented Sep 30, 2023

@mzitnik @BentsiLeviav I tried the latest clickhouse-jdbc-0.5.0-shaded.jar and got a lot of warnings in the console

WARNING: Error when creating APACHE_HTTP_CLIENT, fall back to HTTP_URL_CONNECTION
java.lang.NoClassDefFoundError: org/slf4j/LoggerFactory
  at com.clickhouse.client.internal.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager.<clinit>(PoolingHttpClientConnectionManager.java:108)
  at com.clickhouse.client.http.ApacheHttpConnectionImpl.getDefaultUserAgent(ApacheHttpConnectionImpl.java:214)
  at com.clickhouse.client.http.ClickHouseHttpConnection.getUserAgent(ClickHouseHttpConnection.java:405)
  at com.clickhouse.client.http.ClickHouseHttpConnection.<init>(ClickHouseHttpConnection.java:367)
  at com.clickhouse.client.http.ApacheHttpConnectionImpl.<init>(ApacheHttpConnectionImpl.java:78)
  at com.clickhouse.client.http.ClickHouseHttpConnectionFactory.createConnection(ClickHouseHttpConnectionFactory.java:22)
  at com.clickhouse.client.http.ClickHouseHttpClient.newConnection(ClickHouseHttpClient.java:56)
  at com.clickhouse.client.http.ClickHouseHttpClient.newConnection(ClickHouseHttpClient.java:26)

I found out about this change and when I added to classpath slf4j-api-2.0.9.jar and slf4j-nop-2.0.9.jar then there were no warnings and APACHE_HTTP_CLIENT was used.

Previously the shaded jar contained all needed "shaded" dependencies. I'm wondering if slf4j should also be included as shaded dependencies in the shaded jar to avoid the need to add them explicitly? In our case we would prefer to have just a single shaded jar file which does not depend on any external logging facitilites.

Kind regards,
Raimonds

@BentsiLeviav
Copy link
Contributor Author

Hi @rsim,

Thank you for your comment.

Our APACHE_HTTP_CLIENT is based on httpclient5, which requires SLF4J. We can't shade it as it is a dependency of a dependency.

We would prefer to have just a single shaded JAR file which does not depend on any external logging facilities.

This could be achieved by having a shaded Apache HTTP package that shades the use of SLF4J.

We'll reach out to the Apache HTTP Client team to get more information about this matter. In case they provide us with a shaded version, we'll update it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants