Skip to content

KAFKA-18472: Remove MetadataSupport#18483

Merged
ijuma merged 14 commits intoapache:trunkfrom
m1a2st:KAFKA-18472
Jan 16, 2025
Merged

KAFKA-18472: Remove MetadataSupport#18483
ijuma merged 14 commits intoapache:trunkfrom
m1a2st:KAFKA-18472

Conversation

@m1a2st
Copy link
Copy Markdown
Collaborator

@m1a2st m1a2st commented Jan 10, 2025

Jira: https://issues.apache.org/jira/browse/KAFKA-18472

Committer Checklist (excluded from commit message)

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

@github-actions github-actions Bot added triage PRs from the community core Kafka Broker labels Jan 10, 2025
@m1a2st m1a2st changed the title KAFKA-18472: Remove ZkSupport and cleanup MetadataSupport [WIP]KAFKA-18472: Remove ZkSupport and cleanup MetadataSupport Jan 11, 2025
# Conflicts:
#	core/src/main/scala/kafka/server/KafkaApis.scala
# Conflicts:
#	core/src/main/scala/kafka/server/KafkaApis.scala
@chia7712
Copy link
Copy Markdown
Member

@m1a2st please fix conflicts

# Conflicts:
#	core/src/main/scala/kafka/server/KafkaApis.scala
@m1a2st m1a2st changed the title [WIP]KAFKA-18472: Remove ZkSupport and cleanup MetadataSupport KAFKA-18472: Remove ZkSupport and cleanup MetadataSupport Jan 15, 2025
new DescribeClientQuotasResponse(result)
})
}
val result = metadataCache.asInstanceOf[KRaftMetadataCache]describeClientQuotas(describeClientQuotasRequest.data())
Copy link
Copy Markdown
Member

@ijuma ijuma Jan 15, 2025

Choose a reason for hiding this comment

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

Do we have a JIRA for removing this cast? There should be no reason to do casts anymore since the only implementation we have is this one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is a Jira to remove zkMetadataCache, I think we should also addressed it in this Jira

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good.

@chia7712
Copy link
Copy Markdown
Member

@m1a2st please fix the conflicts

# Conflicts:
#	core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
@github-actions github-actions Bot added the small Small PRs label Jan 15, 2025
@chia7712
Copy link
Copy Markdown
Member

I believe we can eliminate MetadataSupport entirely, as KafkaApis can directly utilize forwardingManager. This would streamline the codebase.

@m1a2st
Copy link
Copy Markdown
Collaborator Author

m1a2st commented Jan 15, 2025

It make sense to me, I will change to remove all MetadataSupport

@m1a2st m1a2st changed the title KAFKA-18472: Remove ZkSupport and cleanup MetadataSupport KAFKA-18472: Remove MetadataSupport Jan 15, 2025
@github-actions github-actions Bot added performance and removed small Small PRs labels Jan 15, 2025
requestHelper.sendMaybeThrottle(request, subscriptionRequest.getErrorResponse(Errors.INVALID_REQUEST.exception))
}
case None =>
info("Received get telemetry client request for zookeeper based cluster")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please revert this change as it is included by #18550?

requestHelper.sendMaybeThrottle(request, pushTelemetryRequest.getErrorResponse(Errors.INVALID_REQUEST.exception))
}
case None =>
info("Received push telemetry client request for zookeeper based cluster")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

// This should never happen as ZK based cluster calls should get rejected earlier itself,
// This should never happen as based cluster calls should get rejected earlier itself,
// but we should handle it gracefully.
info("Received list client metrics resources request for zookeeper based cluster")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

case Some(manager) => manager
case None =>
// The API is not supported when the SharePartitionManager is not defined on the broker
info("Received share acknowledge request for zookeeper based cluster")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

case Some(manager) => manager
case None =>
// The API is not supported when the SharePartitionManager is not defined on the broker
info("Received share fetch request for zookeeper based cluster")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

# Conflicts:
#	core/src/main/scala/kafka/server/KafkaApis.scala
@github-actions github-actions Bot removed the triage PRs from the community label Jan 16, 2025
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

new DescribeClientQuotasResponse(result)
})
}
val result = metadataCache.asInstanceOf[KRaftMetadataCache].describeClientQuotas(describeClientQuotasRequest.data())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A note for someone who sees this - there is a plan to address these casts separately. My suggestion elsewhere is to simply move the relevant methods to the base interface and then the cast is no longer needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for @ijuma reminder, cc @FrankYang0529

@ijuma ijuma merged commit b9ccab4 into apache:trunk Jan 16, 2025
ijuma pushed a commit that referenced this pull request Jan 16, 2025
Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>
pranavt84 pushed a commit to pranavt84/kafka that referenced this pull request Jan 27, 2025
Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>
airlock-confluentinc Bot pushed a commit to confluentinc/kafka that referenced this pull request Jan 27, 2025
Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants