m1a2st commented on code in PR #19050:
URL: https://github.com/apache/kafka/pull/19050#discussion_r2027327151
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -766,7 +766,7 @@ class BrokerServer(
CoreUtils.swallow(dataPlaneRequestHandlerPool.shutdown(), this)
if (dataPlaneRequestProcessor != null)
CoreUtils.swallow(dataPlaneRequestProcessor.close(), this)
- authorizer.foreach(Utils.closeQuietly(_, "authorizer"))
+ authorizerPlugin.foreach(Utils.closeQuietly(_, "authorizer"))
Review Comment:
Should we rename "authorizer" to "authorizer plugin" for clarity? Since we
explicitly close the authorizer in `Plugin#close()`.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -250,7 +251,7 @@ public GroupCoordinatorShard build() {
throw new IllegalArgumentException("TopicPartition must be
set.");
if (groupConfigManager == null)
throw new IllegalArgumentException("GroupConfigManager must be
set.");
- if (authorizer == null)
+ if (authorizerPlugin == null)
Review Comment:
Should we also check `authorizerPlugin.isEmpty()`?
##########
core/src/main/scala/kafka/server/ControllerServer.scala:
##########
@@ -467,7 +470,7 @@ class ControllerServer(
CoreUtils.swallow(quotaManagers.shutdown(), this)
Utils.closeQuietly(controller, "controller")
Utils.closeQuietly(quorumControllerMetrics, "quorum controller metrics")
- authorizer.foreach(Utils.closeQuietly(_, "authorizer"))
+ authorizerPlugin.foreach(Utils.closeQuietly(_, "authorizer"))
Review Comment:
ditto: Should we rename "authorizer" to "authorizer plugin" for clarity?
Since we explicitly close the authorizer in `Plugin#close()`.
##########
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/metadata/KRaftMetadataRequestBenchmark.java:
##########
@@ -94,8 +94,8 @@
@State(Scope.Benchmark)
@Fork(value = 1)
-@Warmup(iterations = 5)
-@Measurement(iterations = 15)
+@Warmup(iterations = 1)
+@Measurement(iterations = 3)
Review Comment:
Just curious, why is this change necessary?
##########
metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizer.java:
##########
@@ -47,7 +54,7 @@
* The standard authorizer which is used in KRaft-based clusters if no other
authorizer is
* configured.
Review Comment:
We can remove KRaft-related keywords to improve this document. because we
only have KRaft based cluster in 4.X.
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTestContext.java:
##########
@@ -505,8 +506,8 @@ public Builder
withShareGroupAssignor(ShareGroupPartitionAssignor shareGroupAssi
return this;
}
- public Builder withAuthorizer(Authorizer authorizer) {
- this.authorizer = Optional.of(authorizer);
+ public Builder withAuthorizerPlugin(Plugin<Authorizer>
authorizerPlugin) {
+ this.authorizerPlugin = Optional.of(authorizerPlugin);
Review Comment:
We set the parameter is `Optional<Plugin<Authorizer>> authorizerPlugin` in
`GroupMetadataManager`, If we consistently pass `Plugin<Authorizer>` into the
builder and wrap it inside the method, can we avoid the need to check for null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
