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

KAFKA-15007: Always update the MetadataPropagator with correct MV. #13732

Merged
merged 3 commits into from May 22, 2023

Conversation

akhileshchg
Copy link
Contributor

Even if the MigrationDriver is not in DUAL_WRITE mode, it should pass the MV changes to the MetadataPropagator. If not, the propagator might not have the right version to send UMR, LISR and StopReplica requests when the migration is in DUAL_WRITE mode.

Even if the MigrationDriver is not in DUAL_WRITE mode, it should pass the MV
changes to the MetadataPropagator.  If not, the propagator might not have the
right version to send UMR, LISR and StopReplica requests when the migration is
in DUAL_WRITE mode.
@mumrah
Copy link
Contributor

mumrah commented May 19, 2023

@akhileshchg what do you think about passing a supplier down to MetadataPropagator rather than the MV value? This way, we'll always be guaranteed to be using the MV in the MetadataImage known to KRaftMigrationDriver.

@mumrah mumrah self-assigned this May 22, 2023
@mumrah mumrah added the kraft label May 22, 2023
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 the patch, @akhileshchg. Just one question inline

@@ -70,7 +68,7 @@ class MigrationPropagator(
val requestBatch = new MigrationPropagatorBatch(
config,
metadataProvider,
() => metadataVersion,
() => _image.features().metadataVersion(),
Copy link
Contributor

Choose a reason for hiding this comment

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

One subtle difference in behavior here is that the MV of the empty image is IBP_3_0_IV1, so the default we previously had of IBP_3_4_IV0 is no longer seen.

However.. I don't think we actually hit these defaults in practice since we won't call the metadata propagator until we've seen at least one metadata publish. @akhileshchg can you confirm if my assumption is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That's correct. For MigrationPropagator to first publish something, it should be called with an associated image. We use this image to setup the correct metadata version required to send the requests to ZkBrokers.

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 the confirmation. LGTM

@mumrah mumrah merged commit 6b95581 into apache:trunk May 22, 2023
1 check failed
cmccabe pushed a commit that referenced this pull request May 26, 2023
…13732)

Use the MetadataVersion from the MetadataImage passed to MetadataPropagator. The ensures the propagator 
sends the right versions of UMR, LISR and StopReplica requests when the migration is in DUAL_WRITE mode.

Reviewers: David Arthur <mumrah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants