Skip to content

Lower Kafka ConsumerConfig log level #14134

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.

Sign up for GitHub

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

Closed
nevillelyh wants to merge 1 commit into from
Closed

Lower Kafka ConsumerConfig log level #14134

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConsumerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
package io.trino.plugin.kafka;

import io.trino.spi.connector.ConnectorSession;
import org.apache.kafka.clients.consumer.ConsumerConfig;
import org.apache.kafka.clients.consumer.KafkaConsumer;

import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;

public interface KafkaConsumerFactory
{
default KafkaConsumer<byte[], byte[]> create(ConnectorSession session)
{
Logger.getLogger(ConsumerConfig.class.getName()).setLevel(Level.WARNING);
Copy link
Copy Markdown
Member

@kokosing kokosing Sep 15, 2022

Choose a reason for hiding this comment

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

We cannot do it like that.

First it would have to be done once per connector initialization so somewhere in the io.trino.plugin.kafka.KafkaConnectorFactory#create.
Also if set logging statically then you would override anything that is set via https://trino.io/docs/current/admin/properties-logging.html.

So I think we need to disable logging in tests. Somewhere here io.trino.plugin.kafka.KafkaQueryRunnerBuilder#build and update docs to mention that it is a good practice to set this in log.properties so your log file is less polluted.

I would do it as two PRs as they require different people for review.

Copy link
Copy Markdown
Member Author

@nevillelyh nevillelyh Sep 15, 2022

Choose a reason for hiding this comment

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

Also if set logging statically then you would override anything that is set via https://trino.io/docs/current/admin/properties-logging.html.
Yeah I was looking for alternatives like a connector level conf file but can't find any. Didn't want to change the default log.properties.

So instead of hard coding it here, we just have to document how to suppress this & hard code setLevel in tests, right?

Copy link
Copy Markdown
Member Author

@nevillelyh nevillelyh Sep 15, 2022

Choose a reason for hiding this comment

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

Also looks like Kafka logging is already suppressed in KafkaQueryRunnerBuilder. So this should be a doc change only?

Copy link
Copy Markdown
Member

@kokosing kokosing Sep 15, 2022

Choose a reason for hiding this comment

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

So this should be a doc change only?

Yes, it looks like that. There also product tests where we can implement this suggestion. So we can avoid poluting logs in product tests.
See io.trino.tests.product.launcher.env.common.Kafka where we could have log.properties file that is used.

Please reach to @andrewdibiasio6 or @lukasz-walkiewicz to help you run product tests for kafka here.

Copy link
Copy Markdown
Member Author

@nevillelyh nevillelyh Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Replacing with these 2 PRs:

return new KafkaConsumer<>(configure(session));
}

Expand Down