-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
KAFKA-17543: Improve and clarify the error message about generated broker IDs in migration #17210
KAFKA-17543: Improve and clarify the error message about generated broker IDs in migration #17210
Conversation
…ion should not be used, it does not have to be disabled) Signed-off-by: Jakub Scholz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scholzj thanks for your patch
@@ -967,7 +967,7 @@ class KafkaConfig private(doLog: Boolean, val props: util.Map[_, _]) | |||
// ZK-based | |||
if (migrationEnabled) { | |||
require(brokerId >= 0, | |||
"broker broker.id.generation.enable is incompatible with migration to ZK. Please disable it before enabling migration") | |||
"broker.id generation is incompatible with ZooKeeper migration. Please stop using it before enabling migration (set broker.id to a value greater or equal to 0).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this path is unrelated to broker.id generation, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure why it is here -> I just updated the message. The test does not trigger it. But to be honest, my Kafka code knowledge is not good enough to understand if under some circumstances it might get here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path and the above one (L850) are both under validateValues
method. The above one is validate for ZK brokers (i.e. process.role is null). And this line is to validate KRaft, or migrating brokers. Both are related to this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just say "please define the broker.id for migration"? It can fail due to undefined broker.if even though we have disabled generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in the error message? Something like Please define the broker.id for ZooKeeper migration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind. Current version is good enough. I will merge it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -967,7 +967,7 @@ class KafkaConfig private(doLog: Boolean, val props: util.Map[_, _]) | |||
// ZK-based | |||
if (migrationEnabled) { | |||
require(brokerId >= 0, | |||
"broker broker.id.generation.enable is incompatible with migration to ZK. Please disable it before enabling migration") | |||
"broker.id generation is incompatible with ZooKeeper migration. Please stop using it before enabling migration (set broker.id to a value greater or equal to 0).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path and the above one (L850) are both under validateValues
method. The above one is validate for ZK brokers (i.e. process.role is null). And this line is to validate KRaft, or migrating brokers. Both are related to this fix.
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
3.9 RC is almost there and this PR should be backport to 3.9, hence I will wait for RC 1 ... |
…oker IDs in migration (#17210) This PR tries to improve the error message when broker.id is set to -1 and ZK migration is enabled. It is not needed to disable the broker.id.generation.enable option. It is sufficient to just not use it (by not setting the broker.id to -1). Reviewers: Chia-Ping Tsai <[email protected]>, Luke Chen <[email protected]>
This PR tries to improve the error message when
broker.id
is set to-1
when migration is enabled. It is not needed to disable thebroker.id.generation.enable
option. It is sufficient to just not use it (by not setting thebroker.id
to-1
). This was discussed inCommitter Checklist (excluded from commit message)