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

improve log description of QuorumController #15926

Merged
merged 4 commits into from
Jun 17, 2024

Conversation

chickenchickenlove
Copy link
Contributor

Motivation

  • QuorumController warn with "Performing controller activation. Loaded ZK migration state of NONE" even if it's de-novo KRaft cluster. That message makes operator confused, and operator might waste time investigating that log. If we add some description to log, operator will be more convenient.

Modification

  • Add a log for de-novo KRaft cluster when zookeeper.metadata.migration.enable=false and the broker version supports KRaft.

Result

@chickenchickenlove
Copy link
Contributor Author

Hey, @mumrah !
I create PR to improve log desrcription when zookeeper.metadata.migration.enable is false.

When you have free time, could you take a look? 🙇‍♂️

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @chickenchickenlove 👍

Left a suggestion for the log message

@@ -165,6 +165,8 @@ static ControllerResult<Void> recordsForNonEmptyLog(
throw new RuntimeException("Should not have ZK migrations enabled on a cluster that was " +
"created in KRaft mode.");
}
logMessageBuilder
.append("since this is a de-novo KRaft cluster.");
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "This is expected because this is a de-novo KRaft cluster." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is better 👍
I make a new commit to apply your comments.
Please take another look, when you have free time 🙇‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mumrah gently ping.
When you have free time, please take another look?
Thanks in advance! 🙇‍♂️

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Left one small style nit. Otherwise, LGTM

@@ -165,6 +165,8 @@ static ControllerResult<Void> recordsForNonEmptyLog(
throw new RuntimeException("Should not have ZK migrations enabled on a cluster that was " +
"created in KRaft mode.");
}
logMessageBuilder
.append("This is expected because this is a de-novo KRaft cluster.");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the unnecessary line break here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mumrah , i make new commit to apply your comments.
However, the CI still is failed.
The changes which i made seems not to be related with failure of CI.
Am i wrong?
image

@mumrah
Copy link
Contributor

mumrah commented Jun 13, 2024

@chickenchickenlove
Copy link
Contributor Author

I started a new build here https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15926/6/

Thanks for your comments.
If there is something to be fixed, please let me know 🙇‍♂️

@mumrah
Copy link
Contributor

mumrah commented Jun 17, 2024

Test failures look unrelated. Merging

@mumrah mumrah merged commit 1a7ba66 into apache:trunk Jun 17, 2024
1 check failed
@chickenchickenlove
Copy link
Contributor Author

Hey, mumrah.
Thanks for your time 🙇‍♂️

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