KAFKA-18708: Move ScramPublisher to metadata module #20468

see-quick · 2025-09-03T13:55:58Z

This PR moves the ScramPublisher class from the server metadata package
to the dedicated metadata module. During refactoring, I found out that I
also need to move the CredentialProvider interface to a more appropriate
location in the server common package because CredentialProvider is in
the server module, and I can't include that module in the metadata
because I would create a circular dependency, i.e.,

server module ←-------------┐ ↓ (depends on). │
(which would make it circular) metadata module------------┘

So I have moved CredentialProvider to server-common module, and
metadata module has already server-common and thus it's resolved.

Reviewers: Mickael Maison mickael.maison@gmail.com

Signed-off-by: see-quick <maros.orsak159@gmail.com>

mimaison

Thanks for the PR. I left a few comments

core/src/main/scala/kafka/network/SocketServer.scala

core/src/main/scala/kafka/server/BrokerServer.scala

core/src/main/scala/kafka/server/ControllerServer.scala

core/src/main/scala/kafka/server/KafkaBroker.scala

core/src/main/scala/kafka/tools/TestRaftServer.scala

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java

server-common/src/main/java/org/apache/kafka/server/common/CredentialProvider.java

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java

Signed-off-by: see-quick <maros.orsak159@gmail.com>

… admins scramMechanism) Signed-off-by: see-quick <maros.orsak159@gmail.com>

mimaison · 2025-09-09T09:57:34Z

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java

            if (scramDelta != null) {
                scramDelta.changes().forEach((mechanism, userChanges) -> {
                    userChanges.forEach((userName, change) -> {
+                        ScramMechanism internalMechanism = ScramMechanism.forMechanismName(mechanism.mechanismName());


Why is this change needed?

ScramMechanism.forMechanismName returns org.apache.kafka.common.security.scram.internals.ScramMechanism and CredentialProvider methods, i.e., removeCredential and updateCredential need org.apache.kafka.common.security.scram.internals.ScramMechanism type instead of org.apache.kafka.clients.admin.ScramMechanism.

scramDelta.changes() using org.apache.kafka.clients.admin.ScramMechanism. That's why I have added that here.

I'd prefer if we keep the existing types. Let's focus this PR on the Scala to Java conversion.

If you think you can do further refactoring, we should do that in a separate PR.

I'd prefer if we keep the existing types. Let's focus this PR on the Scala to Java conversion.

Updated.

Signed-off-by: see-quick <maros.orsak159@gmail.com>

see-quick · 2025-09-10T08:48:13Z

FYI: Those failed tests are not related to PR.

mimaison

LGTM

chia7712

@see-quick thanks for this patch. Sorry for leaving some comments on this merged PR

chia7712 · 2025-09-10T15:03:05Z

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java

+    }
+
+    public void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage) {
+        String deltaName = "MetadataDelta up to " + newImage.highestOffsetAndEpoch().offset();


Could you please create this variable in the catch block? It is only used by the error handler.

I reviewed the PR comparing line by line with the Scala code which had that as well. Not a big deal but we can indeed move that into the catch block.

Looking at the catch block, I wonder if we should have one in FeaturesPublisher. At a glance, it seems metadataVersionOrThrow() could throw, or it's terribly named!

Looking at the catch block, I wonder if we should have one in FeaturesPublisher. At a glance, it seems metadataVersionOrThrow() could throw, or it's terribly named!

I prefer to let the caller handle the exception if all FeaturesPublisher does is call faultHandler.handleFault

My point was consistency. Maybe there's an explanation but looking at these classes they all catch Throwable and run faultHandler.handleFault() apart from FeaturesPublisher

@mimaison you are right. +1 to add catch to FeaturesPublisher

chia7712 · 2025-09-10T15:18:39Z

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java

+        onMetadataUpdate(delta, newImage);
+    }
+
+    public void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage) {


I'm not sure why we need the onMetadataUpdate(MetadataDelta, MetadataImage) method. It is only used by BrokerMetadataPublisher#onMetadataUpdate, but we could just call onMetadataUpdate(MetadataDelta, MetadataImage, LoaderManifest) instead

@see-quick @mimaison WDYT?

I agree I'm not sure why we have onMetadataUpdate(MetadataDelta, MetadataImage). It seems all the callers have a LoaderManifest instance so they could, and probably should, use onMetadataUpdate(MetadataDelta, MetadataImage, LoaderManifest).

I prefer keeping the code "as is" for the Java to Scala conversion. We can tackle this type of refactorings in follow up PRs.

This seems to be the case across most (all?) the *Publisher classes. So maybe a PR (or set of PRs) addressing them all together would make sense.

So maybe a PR (or set of PRs) addressing them all together would make sense.

+1 to "a PR"

Okay, I will take a look at that and address those issues in the next PR :) Thanks @chia7712 @mimaison.

I am considering updating this after all publisher classes have been converted to Java. Or?

Maybe you could fix the ScramPublisher in PR#20522 and leave the other publishers for the migration :)

Maybe you could fix the ScramPublisher in PR#20522 and leave the other publishers for the migration :)

Yeah, that's a good idea. Will do.

This PR is a follow-up from #20468. Basically makes two things: 1. Moves the variable to the catch block as it is used only there. 2. Refactor FeaturesPublisher to handle exceptions the same as ScramPublisher or other publishers :) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> --------- Signed-off-by: see-quick <maros.orsak159@gmail.com>

This PR is a follow-up from apache#20468. Basically makes two things: 1. Moves the variable to the catch block as it is used only there. 2. Refactor FeaturesPublisher to handle exceptions the same as ScramPublisher or other publishers :) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> --------- Signed-off-by: see-quick <maros.orsak159@gmail.com>

see-quick added 3 commits September 3, 2025 15:39

KAFKA-18704: Move ScramPublisher to metadata module

feee50f

Signed-off-by: see-quick <maros.orsak159@gmail.com>

remove the logging

6e97c37

Signed-off-by: see-quick <maros.orsak159@gmail.com>

checkstyle

e61fe27

Signed-off-by: see-quick <maros.orsak159@gmail.com>

github-actions bot added triage PRs from the community core Kafka Broker kraft labels Sep 3, 2025

showuon self-assigned this Sep 4, 2025

see-quick changed the title ~~KAFKA-18704: Move ScramPublisher to metadata module~~ KAFKA-18706: Move ScramPublisher to metadata module Sep 4, 2025

mimaison added the ci-approved label Sep 5, 2025

mimaison reviewed Sep 5, 2025

View reviewed changes

metadata/src/main/java/org/apache/kafka/metadata/publisher/ScramPublisher.java Show resolved Hide resolved

github-actions bot removed the triage PRs from the community label Sep 6, 2025

see-quick added 3 commits September 8, 2025 11:34

update review from Mickael

fa222ac

Signed-off-by: see-quick <maros.orsak159@gmail.com>

checkstyle

2b1c969

Signed-off-by: see-quick <maros.orsak159@gmail.com>

also use proper imports and types within ScramMechanism (internals vs…

c7d0425

… admins scramMechanism) Signed-off-by: see-quick <maros.orsak159@gmail.com>

mimaison reviewed Sep 9, 2025

View reviewed changes

back to current types

9b217f6

Signed-off-by: see-quick <maros.orsak159@gmail.com>

mimaison approved these changes Sep 10, 2025

View reviewed changes

mimaison changed the title ~~KAFKA-18706: Move ScramPublisher to metadata module~~ KAFKA-18708: Move ScramPublisher to metadata module Sep 10, 2025

mimaison merged commit a244565 into apache:trunk Sep 10, 2025
33 of 37 checks passed

chia7712 reviewed Sep 10, 2025

View reviewed changes

see-quick mentioned this pull request Sep 11, 2025

MINOR: Refactor on FeaturesPublisher and ScramPublisher #20522

Merged

KAFKA-18708: Move ScramPublisher to metadata module #20468

KAFKA-18708: Move ScramPublisher to metadata module #20468

Uh oh!

Conversation

see-quick commented Sep 3, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

see-quick Sep 9, 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!

see-quick commented Sep 10, 2025

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chia7712 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!

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!

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!

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!

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!

Uh oh!

see-quick commented Sep 3, 2025 •

edited by github-actions bot

Loading

see-quick Sep 9, 2025 •

edited

Loading