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

Fixes #488: Add logging to the driver config #494

Closed
wants to merge 8 commits into from

Conversation

igorvoltaic
Copy link

@igorvoltaic igorvoltaic commented Sep 8, 2021

Fixes #488

Suggestion to change the log level of Neo4j driver

Proposed Changes

A brief list of proposed changes in order to fix the issue:

  • add SLF4j Logger config to configBuilder in Neo4jService.kt

Copy link
Contributor

@conker84 conker84 left a comment

Choose a reason for hiding this comment

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

Hi @igorvoltaic thank you so much for the contribution, can yo please answer to my question?

@@ -39,7 +40,7 @@ class Neo4jService(private val config: Neo4jSinkConnectorConfig):
private val transactionConfig: TransactionConfig

init {
val configBuilder = Config.builder()
val configBuilder = Config.builder().withLogging(Logging.slf4j())
Copy link
Contributor

Choose a reason for hiding this comment

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

why this approach and not the same of the other PR?

Copy link
Author

@igorvoltaic igorvoltaic Sep 24, 2021

Choose a reason for hiding this comment

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

Hi Andrea! Thanks for taking your time and looking at my PRs! Like I've said in the support ticket that's only because I'm not Java/Kotlin guy and wanted your experienced look at these two approaches. I saw you mentioned you liked the other approach, I can definitely port it to this PR. Also I wasn't really sure which is the best way because offical neo4j driver API docs literally say: This logging method is the preferred one and relies on the SLF4J implementation available in the classpath or modulepath

Also it worth mentioning that ideally the connector logs should be managed in the similar manner the official docs suggests it. Where they could be configured via container env variables picked up by native confluent kafka-connect env tools

log4j.logger.org.apache.kafka.connect.runtime.WorkerSinkTask=DEBUG
log4j.logger.org.apache.kafka.connect=DEBUG

Please let me know if you still want to port #493? Thanks again!

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the code of the java driver:

https://github.com/neo4j/neo4j-java-driver/blob/4.3.4/driver/src/main/java/org/neo4j/driver/Config.java#L272

It uses the same API that you used in the other PR.

So maybe I misunderstood the goal of this and other PR. Do you want to provide a config option for the log level of the java driver? Or?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the link! Yeah, I do, I've ported the PR

Copy link
Contributor

@conker84 conker84 left a comment

Choose a reason for hiding this comment

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

@igorvoltaic please consider to apply all my comments to branch 4.1 in particular in Neo4jConnectorConfig.kt as requested here:

#493 (comment)

Branch 4.1 contains the code of Kafka Connect Neo4j Connecto 2.0.0 that we released two weeks ago, and is the place where al new features will be implemented. In order to use this cool contribution please port these (with the requested changes) to branch 4.1

Thank you so much!!!

@igorvoltaic igorvoltaic changed the title add SLF4j logging to the driver config Fixes #488: Add logging to the driver config Sep 27, 2021
@igorvoltaic
Copy link
Author

Implemented in #502, version 4.1

@igorvoltaic igorvoltaic closed this Oct 1, 2021
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.

2 participants