-
Notifications
You must be signed in to change notification settings - Fork 14.5k
MINOR: Cleanup Core Module- Scala Modules (2/n) #19510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjhajharia, I think we can do more in this PR.
- mutable.HashSet -> util.Set.of
kafka/core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
Lines 10731 to 10733 in 2cd733c
val resources = new mutable.HashSet[String] resources.add("test1") resources.add("test2") - util.HashMap -> util.Map.of
kafka/core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
Lines 5511 to 5523 in 2cd733c
val erroneousPartitions: util.Map[TopicIdPartition, ShareFetchResponseData.PartitionData] = new util.HashMap() erroneousPartitions.put( tp2, new ShareFetchResponseData.PartitionData() .setPartitionIndex(1) .setErrorCode(Errors.UNKNOWN_TOPIC_OR_PARTITION.code) ) erroneousPartitions.put( tp3, new ShareFetchResponseData.PartitionData() .setPartitionIndex(0) .setErrorCode(Errors.UNKNOWN_TOPIC_OR_PARTITION.code) )
Thanks @FrankYang0529 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also remove the operations of asScala
followed by asJava
.
For example:
kafka/core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
Lines 3016 to 3018 in c009eed
actions.asScala.map { action => | |
acls.getOrElse(action.resourcePattern.name, AuthorizationResult.DENIED) | |
}.asJava |
Thanks @frankvicky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this patch, LGTM
Hey @chia7712 , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjhajharia : LGTM assuming CI pass
Thanks @frankvicky |
@@ -9332,7 +9316,7 @@ class KafkaApisTest extends Logging { | |||
assertEquals(clusterId, describeClusterResponse.data.clusterId) | |||
assertEquals(8096, describeClusterResponse.data.clusterAuthorizedOperations) | |||
assertEquals(util.Set.copyOf(metadataCache.getAliveBrokerNodes(plaintextListener)), | |||
describeClusterResponse.nodes.asScala.values.toSet.asJava) | |||
new util.HashSet(describeClusterResponse.nodes.values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Left one minor.
new util.HashSet(describeClusterResponse.nodes.values)) | |
util.Set.copyOf(describeClusterResponse.nodes.values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup.
There are many occurrences of this pattern:
val topicNames = new util.HashMap[Uuid, String]()
topicNames.put(topicId1, "foo1")
topicNames.put(topicId2, "foo2")
But in many of those cases, no additional topics are added later, so they can all be replaced with:
val topicNames = util.Map.of(topicId1, "foo1", topicId2, "foo2")
For example:
https://github.com/apache/kafka/blob/trunk/core/src/test/scala/unit/kafka/server/KafkaApisTest.scala#L7166C2-L7168C37
@@ -9332,7 +9316,7 @@ class KafkaApisTest extends Logging { | |||
assertEquals(clusterId, describeClusterResponse.data.clusterId) | |||
assertEquals(8096, describeClusterResponse.data.clusterAuthorizedOperations) | |||
assertEquals(util.Set.copyOf(metadataCache.getAliveBrokerNodes(plaintextListener)), | |||
describeClusterResponse.nodes.asScala.values.toSet.asJava) | |||
new util.HashSet(describeClusterResponse.nodes.values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use util.Set.copyOf
?
Thanks @mingyen066 and @Yunyung for the review |
Gentle reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Now that Kafka Brokers support Java 17, this PR makes some changes in
core module. The changes in this PR are limited to only the Scala files
in the Core module's tests. The unit tests module is still pending. It
shall follow next. The changes mostly include:
Arrays.asList() are replaced with List.of()
with Map.of()
To be clear, the directories being targeted in this PR are:
KafkaApisTest.scala
Reviewers: PoAn Yang [email protected], Ming-Yen Chung [email protected], Yung [email protected], TengYao Chi [email protected], Ken Huang [email protected]