chia7712 commented on code in PR #20346:
URL: https://github.com/apache/kafka/pull/20346#discussion_r2292066790
##########
metadata/src/main/java/org/apache/kafka/image/ScramImage.java:
##########
@@ -105,56 +103,39 @@ public DescribeUserScramCredentialsResponseData
describe(DescribeUserScramCreden
DescribeUserScramCredentialsResponseData retval = new
DescribeUserScramCredentialsResponseData();
- for (Map.Entry<String, Boolean> user : uniqueUsers.entrySet()) {
+ for (Entry<String, Boolean> user : uniqueUsers.entrySet()) {
DescribeUserScramCredentialsResult result = new
DescribeUserScramCredentialsResult().setUser(user.getKey());
if (!user.getValue()) {
boolean dataFound = false;
List<CredentialInfo> credentialInfos = new ArrayList<>();
- for (Map.Entry<ScramMechanism, Map<String,
ScramCredentialData>> mechanismsEntry : mechanisms.entrySet()) {
+ for (Entry<ScramMechanism, Map<String, ScramCredentialData>>
mechanismsEntry : mechanisms.entrySet()) {
Review Comment:
Using `var` makes it clearer.
##########
metadata/src/main/java/org/apache/kafka/metadata/placement/TopicAssignment.java:
##########
@@ -18,16 +18,13 @@
package org.apache.kafka.metadata.placement;
import java.util.List;
-import java.util.Objects;
/**
* The topic assignment.
- *
+ * <p>
* This class is immutable. It's internal state does not change.
*/
-public class TopicAssignment {
- private final List<PartitionAssignment> assignments;
-
+public record TopicAssignment(List<PartitionAssignment> assignments) {
public TopicAssignment(List<PartitionAssignment> assignments) {
Review Comment:
```java
public TopicAssignment {
assignments = List.copyOf(assignments);
}
```
##########
metadata/src/main/java/org/apache/kafka/image/ScramImage.java:
##########
@@ -105,56 +103,39 @@ public DescribeUserScramCredentialsResponseData
describe(DescribeUserScramCreden
DescribeUserScramCredentialsResponseData retval = new
DescribeUserScramCredentialsResponseData();
- for (Map.Entry<String, Boolean> user : uniqueUsers.entrySet()) {
+ for (Entry<String, Boolean> user : uniqueUsers.entrySet()) {
Review Comment:
ditto
##########
metadata/src/main/java/org/apache/kafka/metadata/placement/TopicAssignment.java:
##########
@@ -38,22 +35,4 @@ public TopicAssignment(List<PartitionAssignment>
assignments) {
public List<PartitionAssignment> assignments() {
Review Comment:
this is not necessary after switching to a `record` class, right?
##########
metadata/src/main/java/org/apache/kafka/metadata/FinalizedControllerFeatures.java:
##########
@@ -48,30 +44,11 @@ public Set<String> featureNames() {
return featureMap.keySet();
}
- public Map<String, Short> featureMap() {
- return featureMap;
- }
-
- public long epoch() {
- return epoch;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(featureMap, epoch);
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof FinalizedControllerFeatures other)) return false;
- return featureMap.equals(other.featureMap) && epoch == other.epoch;
- }
-
@Override
public String toString() {
Review Comment:
This seems to be equivalent to the generated `toString`?
##########
metadata/src/main/java/org/apache/kafka/image/MetadataVersionChange.java:
##########
@@ -21,57 +21,23 @@
import java.util.Objects;
-
/**
* A change in the MetadataVersion.
*/
-public final class MetadataVersionChange {
- private final MetadataVersion oldVersion;
- private final MetadataVersion newVersion;
-
+public record MetadataVersionChange(MetadataVersion oldVersion,
MetadataVersion newVersion) {
public MetadataVersionChange(
- MetadataVersion oldVersion,
- MetadataVersion newVersion
+ MetadataVersion oldVersion,
+ MetadataVersion newVersion
) {
this.oldVersion = Objects.requireNonNull(oldVersion);
Review Comment:
```java
public MetadataVersionChange{
Objects.requireNonNull(oldVersion);
Objects.requireNonNull(newVersion);
}
```
Also, please add unit test for it
##########
metadata/src/main/java/org/apache/kafka/image/ScramImage.java:
##########
@@ -39,14 +39,12 @@
/**
* Represents the SCRAM credentials in the metadata image.
- *
+ * <p>
* This class is thread-safe.
*/
-public final class ScramImage {
+public record ScramImage(Map<ScramMechanism, Map<String, ScramCredentialData>>
mechanisms) {
public static final ScramImage EMPTY = new ScramImage(Map.of());
- private final Map<ScramMechanism, Map<String, ScramCredentialData>>
mechanisms;
-
public ScramImage(Map<ScramMechanism, Map<String, ScramCredentialData>>
mechanisms) {
Review Comment:
```java
public ScramImage {
mechanisms = Collections.unmodifiableMap(mechanisms);
}
```
--
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]
