chia7712 commented on code in PR #20180:
URL: https://github.com/apache/kafka/pull/20180#discussion_r2274207213
##########
server/src/main/java/org/apache/kafka/server/config/ClientQuotaManagerConfig.java:
##########
@@ -15,6 +15,7 @@
* limitations under the License.
*/
package org.apache.kafka.server.config;
+
public class ClientQuotaManagerConfig {
Review Comment:
Could you please convert it to `record` class?
##########
server/src/main/java/org/apache/kafka/server/quota/ClientSensors.java:
##########
@@ -43,24 +39,12 @@ public ClientSensors(Map<String, String> metricTags,
this.throttleTimeSensor = Objects.requireNonNull(throttleTimeSensor);
}
- public Map<String, String> metricTags() {
- return metricTags;
- }
-
- public Sensor quotaSensor() {
- return quotaSensor;
- }
-
- public Sensor throttleTimeSensor() {
- return throttleTimeSensor;
- }
-
@Override
public String toString() {
return "ClientSensors{" +
- "metricTags=" + metricTags +
- ", quotaSensor=" + quotaSensor +
- ", throttleTimeSensor=" + throttleTimeSensor +
- '}';
+ "metricTags=" + metricTags +
Review Comment:
Do we really need those custom `toString` methods? it seems to me that the
generated `toString` is already useful enough
##########
server/src/test/java/org/apache/kafka/network/SocketServerConfigsTest.java:
##########
@@ -36,7 +36,7 @@ public void testDefaultNameToSecurityProto() {
new ListenerName("SASL_PLAINTEXT"),
SecurityProtocol.SASL_PLAINTEXT,
new ListenerName("SASL_SSL"), SecurityProtocol.SASL_SSL
);
- assertEquals(expected,
SocketServerConfigs.DEFAULT_NAME_TO_SECURITY_PROTO);
+ assertEquals(SocketServerConfigs.DEFAULT_NAME_TO_SECURITY_PROTO,
expected);
Review Comment:
the origin order is already correct, shouldn't it?
##########
server/src/main/java/org/apache/kafka/server/purgatory/DelayedDeleteRecords.java:
##########
@@ -57,7 +56,7 @@ public DelayedDeleteRecords(long delayMs,
Consumer<Map<TopicPartition,
DeleteRecordsPartitionResult>> responseCallback) {
super(delayMs);
this.onAcksPending = onAcksPending;
- this.deleteRecordsStatus =
Collections.unmodifiableMap(deleteRecordsStatus);
+ this.deleteRecordsStatus = Map.copyOf(deleteRecordsStatus);
Review Comment:
Is the deep copy necessary for now? or we could revisit this code after
rewriting the `ReplicaManager` by java
##########
server/src/main/java/org/apache/kafka/network/SocketServerConfigs.java:
##########
@@ -175,15 +173,12 @@ public class SocketServerConfigs {
private static final Pattern URI_PARSE_REGEXP = Pattern.compile(
"^(.*)://\\[?([0-9a-zA-Z\\-%._:]*)\\]?:(-?[0-9]+)");
- public static final Map<ListenerName, SecurityProtocol>
DEFAULT_NAME_TO_SECURITY_PROTO;
-
- static {
- HashMap<ListenerName, SecurityProtocol> nameToSecurityProtocol = new
HashMap<>();
- for (SecurityProtocol securityProtocol : SecurityProtocol.values()) {
-
nameToSecurityProtocol.put(ListenerName.forSecurityProtocol(securityProtocol),
securityProtocol);
- }
- DEFAULT_NAME_TO_SECURITY_PROTO =
Collections.unmodifiableMap(nameToSecurityProtocol);
- }
+ public static final Map<ListenerName, SecurityProtocol>
DEFAULT_NAME_TO_SECURITY_PROTO =
+ Map.copyOf(Arrays.stream(SecurityProtocol.values())
Review Comment:
we could use `toUnmodifiableMap` to avoid redundant deep copy
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
