-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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-15860: ControllerRegistration must be written out to the metadata image #14807
Conversation
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.
Thanks for the fix @cmccabe . I had a question about the tests.
setMetadataVersion(MetadataVersion.IBP_3_6_IV2). | ||
setLossHandler(loss -> lossString.compareAndSet("controller registration data", loss.loss())). | ||
build()); | ||
assertEquals("", lossString.get()); |
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.
Does this pass? Looking at MetadataVersion
, controller registration support was added in 3.7-IV0
. I would expect that since this test sets the MV to 3.6-IV2
, this test would call the loss handler.
Can we also add a test where the controller registration record is included to the snapshot?
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.
Sorry, I forgot to push the latest version. Should be fixed now.
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.
Thanks for the changes @cmccabe
setMetadataVersion(MetadataVersion.IBP_3_6_IV2). | ||
setLossHandler(loss -> lossString.compareAndSet("", loss.loss())). | ||
build()); | ||
assertEquals("controller registration data", lossString.get()); |
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.
Thanks for fixing this test. I think we are still missing a test that shows that a ClusterImage
with controller registration generates a snapshot with RegisterControllerRecords
if the metadata version is 3.7-IV0
. Can we add that test, if you agree? If not, can you point me to that test?
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.
Both of the unit tests I added in this PR verify that a ClusterImage
with controller registration generates a snapshot with RegisterControllerRecords
if the metadata version is 3.7-IV0
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
…ta image (apache#14807) The ControllerRegistration records added in KIP-919 should be written out to the metadata image, not just the log. Reviewers: José Armando García Sancio <jsancio@apache.org>
…ta image (apache#14807) The ControllerRegistration records added in KIP-919 should be written out to the metadata image, not just the log. Reviewers: José Armando García Sancio <jsancio@apache.org>
…ta image (apache#14807) The ControllerRegistration records added in KIP-919 should be written out to the metadata image, not just the log. Reviewers: José Armando García Sancio <jsancio@apache.org>
The ControllerRegistration records added in KIP-919 should be written out to the metadata
image, not just the log.