KAFKA-18708: Move ScramPublisher to metadata module #20468
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.metadata.publisher; | ||
|
|
||
| import org.apache.kafka.image.MetadataDelta; | ||
| import org.apache.kafka.image.MetadataImage; | ||
| import org.apache.kafka.image.loader.LoaderManifest; | ||
| import org.apache.kafka.image.publisher.MetadataPublisher; | ||
| import org.apache.kafka.server.common.CredentialProvider; | ||
| import org.apache.kafka.server.fault.FaultHandler; | ||
|
|
||
| import java.util.Optional; | ||
|
|
||
| public class ScramPublisher implements MetadataPublisher { | ||
| private final int nodeId; | ||
| private final FaultHandler faultHandler; | ||
| private final String nodeType; | ||
| private final CredentialProvider credentialProvider; | ||
|
|
||
| public ScramPublisher(int nodeId, FaultHandler faultHandler, String nodeType, CredentialProvider credentialProvider) { | ||
| this.nodeId = nodeId; | ||
| this.faultHandler = faultHandler; | ||
| this.nodeType = nodeType; | ||
| this.credentialProvider = credentialProvider; | ||
| } | ||
|
|
||
| @Override | ||
| public final String name() { | ||
| return "ScramPublisher " + nodeType + " id=" + nodeId; | ||
| } | ||
|
|
||
| @Override | ||
| public void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage, LoaderManifest manifest) { | ||
| onMetadataUpdate(delta, newImage); | ||
| } | ||
|
|
||
| public void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage) { | ||
|
There was a problem hiding this comment. I'm not sure why we need the @see-quick @mimaison WDYT? There was a problem hiding this comment. I agree I'm not sure why we have I prefer keeping the code "as is" for the Java to Scala conversion. We can tackle this type of refactorings in follow up PRs. There was a problem hiding this comment. This seems to be the case across most (all?) the There was a problem hiding this comment.
+1 to "a PR" |
||
| String deltaName = "MetadataDelta up to " + newImage.highestOffsetAndEpoch().offset(); | ||
|
There was a problem hiding this comment. Could you please create this variable in the catch block? It is only used by the error handler. There was a problem hiding this comment. 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 There was a problem hiding this comment.
I prefer to let the caller handle the exception if all There was a problem hiding this comment. My point was consistency. Maybe there's an explanation but looking at these classes they all catch There was a problem hiding this comment. @mimaison you are right. +1 to add catch to |
||
| try { | ||
| // Apply changes to SCRAM credentials. | ||
| Optional.ofNullable(delta.scramDelta()).ifPresent(scramDelta -> { | ||
see-quick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| scramDelta.changes().forEach((mechanism, userChanges) -> { | ||
| userChanges.forEach((userName, change) -> { | ||
|
Comment on lines
+56
to
+57
There was a problem hiding this comment. In both cases, the body of the block is a single expression, so we can remove the brackets There was a problem hiding this comment. Also, this could be added to checkstyle, I assume. Updated. |
||
| if (change.isPresent()) { | ||
| credentialProvider.updateCredential(mechanism, userName, change.get().toCredential()); | ||
| } else { | ||
| credentialProvider.removeCredentials(mechanism, userName); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } catch (Throwable t) { | ||
| faultHandler.handleFault("Uncaught exception while publishing SCRAM changes from " + deltaName, t); | ||
| } | ||
| } | ||
| } | ||
see-quick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
see-quick marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.