Skip to content

KAFKA-10340: Improve the trace logging under connector based topic creation #9149

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

Merged
kkonstantine merged 5 commits into from
Oct 15, 2020
Merged

KAFKA-10340: Improve the trace logging under connector based topic creation #9149

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2a8d62
KAFKA-10340: improve the logging to help user know what is going on
showuon Aug 10, 2020
b1131ff
Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-10340
showuon Sep 12, 2020
fb77171
KAFKA-10340: address reviewer's comment to append to trace level log
showuon Sep 21, 2020
ed949d7
Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-10340
showuon Sep 21, 2020
094ee6a
KAFKA-10340: improve the logging
showuon Oct 15, 2020
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
7 changes: 5 additions & 2 deletions connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSourceTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ private boolean sendRecords() {
continue;
}

log.trace("{} Appending record with key {}, value {}", this, record.key(), record.value());
log.trace("{} Appending record to the topic {} with key {}, value {}", this, record.topic(), record.key(), record.value());
// We need this queued first since the callback could happen immediately (even synchronously in some cases).
// Because of this we need to be careful about handling retries -- we always save the previously attempted
// record as part of toSend and need to use a flag to track whether we should actually add it to the outstanding
Expand Down Expand Up @@ -409,6 +409,9 @@ private boolean sendRecords() {
// RegexRouter) topic creation can not be batched for multiple topics
private void maybeCreateTopic(String topic) {
if (!topicCreation.isTopicCreationRequired(topic)) {
log.trace("The topic creation setting is disabled or the topic name {} is already in the topic cache." +

@kkonstantine kkonstantine Sep 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is going to be very repetitive after the first time during which the topic might be created.

What's the hint we are trying to extract here?

@showuon showuon Sep 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@kkonstantine , thanks for the comment, what I'm trying to do here is to let the user know if the creating topic pending for a long time without response (default timeout is 30 or 60 secs), we let user know earlier that this situation might happen, and it could because the auto.create.topics.enable is disabled.

@kkonstantine kkonstantine Oct 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.trace("The topic creation setting is disabled or the topic name {} is already in the topic cache." +
log.trace("Topic creation by the connector is disabled or the topic {} was previously created." +

"If the topic doesn't exist, we'll rely on the auto.create.topics.enable setting in broker side " +
"to see if the topic can be auto created or not", topic);

@kkonstantine kkonstantine Oct 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"If the topic doesn't exist, we'll rely on the auto.create.topics.enable setting in broker side " +
"to see if the topic can be auto created or not", topic);
"If auto.create.topics.enable is enabled on the broker, the topic will be created with " +
"default settings", topic);

return;
}
log.info("The task will send records to topic '{}' for the first time. Checking "
Expand All @@ -430,7 +433,7 @@ private void maybeCreateTopic(String topic) {
log.info("Created topic '{}' using creation group {}", newTopic, topicGroup);
} else {
log.warn("Request to create new topic '{}' failed", topic);
throw new ConnectException("Task failed to create new topic " + topic + ". Ensure "
throw new ConnectException("Task failed to create new topic " + newTopic + ". Ensure "

@showuon showuon Aug 10, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

should be newTopic here for error message.

@kkonstantine kkonstantine Sep 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are referring to the inclusion of topic properties besides the topic name. Yeah that's fine.

+ "that the task is authorized to create topics or that the topic exists and "
+ "restart the task");
}
Expand Down