KAFKA-18373: Remove ZkMetadataCache by FrankYang0529 · Pull Request #18553 · apache/kafka

FrankYang0529 · 2025-01-15T14:15:28Z

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2025-01-15T14:54:37Z

We should also remove casts to the KRaft metadata cache as part of this JIRA or a file a separate issue for that.

FrankYang0529 · 2025-01-15T15:27:15Z

We should also remove casts to the KRaft metadata cache as part of this JIRA or a file a separate issue for that.

Yes, I will do it in this PR. Thanks for the reminder.

chia7712 · 2025-01-15T15:38:43Z

@FrankYang0529 please fix the conflicts

chia7712 · 2025-01-15T15:46:56Z

We should also remove casts to the KRaft metadata cache as part of this JIRA or a file a separate issue for that.

If MetadataCache is extensively used throughout the codebase, replacing all instances with KRaftMetadataCache within this single PR could result in a significant and potentially disruptive change. Maybe we should focus on modifying only the directly related paths and files, minimizing the scope of this PR.

FrankYang0529 · 2025-01-15T15:49:03Z

We should also remove casts to the KRaft metadata cache as part of this JIRA or a file a separate issue for that.

Yes, I will do it in this PR. Thanks for the reminder.

Sorry, I did some quick survey. I would like to make sure we're on the same page first. Do we want to remove MetadataCache interface? I create anothre Jira for followup, so this discussion doesn't block this PR. Thanks.

https://issues.apache.org/jira/browse/KAFKA-18555

TaiJuWu · 2025-01-15T16:18:50Z

-  // TODO: support raft code?
-  private var metadataCache = new ZkMetadataCache(0, MetadataVersion.latestTesting(), BrokerFeatures.createEmpty())
-  metadataCache.updateMetadata(0, updateMetadataRequest)
+  private val metadataDelta = new MetadataDelta(MetadataImage.EMPTY)


Should we need this? it more like zk logic.
I have a similar fix https://github.com/apache/kafka/pull/18354/files

I don't understand the question - what do you mean it's zk logic?

I believe @TaiJuWu is suggesting that this data was previously used by zk-related tests. Since these zk-related tests have already been removed, there is no longer a need to set this data.

That seems a bit odd. This class is testing functionality that is independent of zk. In such cases, we have to be very careful that we're deleting tests that are actually zk specific versus testing functionality that is general but were never converted to the kraft way.

My bad. previous what I think was this setting is test for IBP2.6 but it was deleted by #18468
After checking (checkout to 3.9 and deleted metadataCache.updateMetadata), I found all tests are passed so I am not sure what it is for.

OK, so this is perhaps just dead code (zk or otherwise). That's a nice catch. :)

ijuma · 2025-01-15T16:46:09Z

No, we should not change all the codebase to use the Kraft metadata cache. Instead, we should move the relevant methods from the Kraft implementation to the interface so we don't need to cast - that should not be a large change (but someone needs to check).

ijuma · 2025-01-15T16:47:14Z

Apologies for closing accidentally - I reopened.

Signed-off-by: PoAn Yang <payang@apache.org>

ijuma · 2025-01-16T15:03:44Z

-
-import scala.jdk.CollectionConverters._
-
-class FinalizedFeatureCacheTest {


Don't we need this with kraft?

Yes, in this test, most of cases test ZkMetadataCache#updateFeaturesOrThrow. IIUC, in KRaft, it receives update via setImage, so it doesn't have similar cases as ZkMetadataCache.

OK, thanks.

mimaison

Thanks for the PR. It looks good overall, I just left a couple of minor suggestions.

mimaison · 2025-01-16T17:47:29Z

 import org.mockito.{ArgumentCaptor, ArgumentMatchers, Mockito}

-import java.util.{Collections, Optional, OptionalInt, OptionalLong}
+import java.util.{Optional, OptionalInt, OptionalLong}


It looks like Map imported just below is unused, so we can delete it.

mimaison · 2025-01-16T17:50:24Z

-import org.apache.kafka.server.BrokerFeatures
+import org.apache.kafka.image.{MetadataDelta, MetadataImage, MetadataProvenance}
 import org.apache.kafka.server.config.ReplicationConfigs
-import org.apache.kafka.server.common.{MetadataVersion, OffsetAndEpoch}


scala.collection.Map imported below seems unused, can we delete it too?

Signed-off-by: PoAn Yang <payang@apache.org>

mimaison

LGTM

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>

mimaison · 2025-01-17T11:01:16Z

Applied to 4.0 too: be5b5f3

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>

github-actions Bot added the core Kafka Broker label Jan 15, 2025

FrankYang0529 marked this pull request as draft January 15, 2025 14:17

FrankYang0529 force-pushed the KAFKA-18373 branch from 2515937 to b6c67f6 Compare January 15, 2025 15:45

FrankYang0529 marked this pull request as ready for review January 15, 2025 15:45

TaiJuWu reviewed Jan 15, 2025

View reviewed changes

ijuma closed this Jan 15, 2025

ijuma reopened this Jan 15, 2025

FrankYang0529 force-pushed the KAFKA-18373 branch 2 times, most recently from 5cf16eb to f3d7a74 Compare January 16, 2025 11:05

KAFKA-18373: Remove ZkMetadataCache

df2ae15

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the KAFKA-18373 branch from f3d7a74 to df2ae15 Compare January 16, 2025 13:49

ijuma reviewed Jan 16, 2025

View reviewed changes

ijuma mentioned this pull request Jan 16, 2025

MINOR: Remove casts to KRaftMetadataCache #18579

Merged

3 tasks

mimaison reviewed Jan 16, 2025

View reviewed changes

FrankYang0529 added 3 commits January 17, 2025 09:06

Merge branch 'trunk' into KAFKA-18373

23cc862

address comment

f6e7a45

Signed-off-by: PoAn Yang <payang@apache.org>

Merge branch 'trunk' into KAFKA-18373

25a1b51

mimaison approved these changes Jan 17, 2025

View reviewed changes

mimaison merged commit 5a1fb15 into apache:trunk Jan 17, 2025

mimaison pushed a commit that referenced this pull request Jan 17, 2025

KAFKA-18373: Remove ZkMetadataCache (#18553)

be5b5f3

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>

FrankYang0529 deleted the KAFKA-18373 branch January 18, 2025 01:28

chia7712 mentioned this pull request Jan 19, 2025

KAFKA-18429 Remove ZkFinalizedFeatureCache and StateChangeFailedException #18616

Merged

3 tasks

pranavt84 pushed a commit to pranavt84/kafka that referenced this pull request Jan 27, 2025

KAFKA-18373: Remove ZkMetadataCache (apache#18553)

119123d

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>

airlock-confluentinc Bot pushed a commit to confluentinc/kafka that referenced this pull request Jan 27, 2025

KAFKA-18373: Remove ZkMetadataCache (apache#18553)

78fbfbe

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>

manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025

KAFKA-18373: Remove ZkMetadataCache (apache#18553)

1923d60

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>


		import scala.jdk.CollectionConverters._

		class FinalizedFeatureCacheTest {

Conversation

FrankYang0529 commented Jan 15, 2025

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Jan 15, 2025

Uh oh!

FrankYang0529 commented Jan 15, 2025

Uh oh!

chia7712 commented Jan 15, 2025

Uh oh!

chia7712 commented Jan 15, 2025

Uh oh!

FrankYang0529 commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TaiJuWu Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrankYang0529 commented Jan 15, 2025 •

edited

Loading

TaiJuWu Jan 15, 2025 •

edited

Loading

ijuma Jan 16, 2025 •

edited

Loading

TaiJuWu Jan 16, 2025 •

edited

Loading

ijuma commented Jan 15, 2025 •

edited

Loading