MINOR: Eliminate warnings for AdminUtils #16470

LoganZhuZzz · 2024-06-27T09:56:12Z

Minor modification, eliminate warnings for AdminUtils.

gongxuanzhang · 2024-06-27T10:08:57Z

BTW,I don't think you pulled the latest code.
before your commit,please run ./gradlew checkstyleMain checkstyleTest spotlessCheck to ensure code style

LoganZhuZzz · 2024-06-27T10:36:10Z

BTW,I don't think you pulled the latest code. before your commit,please run ./gradlew checkstyleMain checkstyleTest spotlessCheck to ensure code style

Thanks for that, I will use the latest code and ensure code style

gongxuanzhang · 2024-06-27T10:03:47Z

server-common/src/main/java/org/apache/kafka/admin/AdminUtils.java

        int currentPartitionId = Math.max(0, startPartitionId);
-        int nextReplicaShift = fixedStartIndex >= 0 ? fixedStartIndex : RAND.nextInt(brokerList.size());
+        int nextReplicaShift = determineIndex(fixedStartIndex, brokerList.size());


I don't think we need to refactor this function,the original code is straightforward enough

gongxuanzhang · 2024-06-27T15:59:16Z

LGTM,please take a look,thanks @chia7712

chia7712

@zeq96 thanks for patch

chia7712 · 2024-06-27T17:42:08Z

server-common/src/main/java/org/apache/kafka/admin/AdminUtils.java

@@ -166,7 +166,7 @@ private static Map<Integer, List<Integer>> assignReplicasToBrokersRackAware(int
                                                                                int fixedStartIndex,
                                                                                int startPartitionId) {
        Map<Integer, String> brokerRackMap = new HashMap<>();
-        brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.get()));
+        brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.orElse(null)));


Is it expected that rack will be empty

Is it expected that rack will be empty

Before calling this function, the parameter brokerMetadatas has already been checked. If rack is empty, an AdminOperationException will be thrown. However, my IDE still issues a warning, so I made an equivalent replacement. Next time, I will ignore this situation.

Before calling this function, the parameter brokerMetadatas has already been checked. If rack is empty, an AdminOperationException will be thrown. However, my IDE still issues a warning, so I made an equivalent replacement. Next time, I will ignore this situation.

How about moving the check into assignReplicasToBrokersRackAware? for example:

brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.orElseThrow(() -> new AdminOperationException("Not all brokers have rack information for replica rack aware assignment."))));

Before calling this function, the parameter brokerMetadatas has already been checked. If rack is empty, an AdminOperationException will be thrown. However, my IDE still issues a warning, so I made an equivalent replacement. Next time, I will ignore this situation.

How about moving the check into assignReplicasToBrokersRackAware? for example:

brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.orElseThrow(() -> new AdminOperationException("Not all brokers have rack information for replica rack aware assignment."))));

great suggestion, I will do it

gongxuanzhang · 2024-06-28T09:33:41Z

server-common/src/main/java/org/apache/kafka/admin/AdminUtils.java

@@ -128,12 +128,10 @@ public static Map<Integer, List<Integer>> assignReplicasToBrokers(Collection<Bro
            throw new InvalidReplicationFactorException("Replication factor: " + replicationFactor + " larger than available brokers: " + brokerMetadatas.size() + ".");
        if (brokerMetadatas.stream().noneMatch(b -> b.rack.isPresent()))
            return assignReplicasToBrokersRackUnaware(nPartitions, replicationFactor, brokerMetadatas.stream().map(b -> b.id).collect(Collectors.toList()), fixedStartIndex,
-                startPartitionId);
+                    startPartitionId);


please revert unrelated changes

gongxuanzhang · 2024-06-28T09:33:54Z

server-common/src/main/java/org/apache/kafka/admin/AdminUtils.java

@@ -196,7 +194,7 @@ private static Map<Integer, List<Integer>> assignReplicasToBrokersRackAware(int
                    //    that do not have any replica, or
                    // 2. the broker has already assigned a replica AND there is one or more brokers that do not have replica assigned
                    if ((!racksWithReplicas.contains(rack) || racksWithReplicas.size() == numRacks)
-                        && (!brokersWithReplicas.contains(broker) || brokersWithReplicas.size() == numBrokers)) {
+                            && (!brokersWithReplicas.contains(broker) || brokersWithReplicas.size() == numBrokers)) {


gongxuanzhang · 2024-06-28T09:36:32Z

You can turn this setting off if you are troubled by IDEA's automatic modification of blank line

LoganZhuZzz · 2024-06-28T09:40:39Z

You can turn this setting off if you are troubled by IDEA's automatic modification of blank line

Thanks for that

chia7712

@LoganZhuZzz thanks for your patch. the change LGTM, but it would be nice to have a new test

chia7712 · 2024-06-29T17:02:47Z

server-common/src/main/java/org/apache/kafka/admin/AdminUtils.java

@@ -166,7 +164,7 @@ private static Map<Integer, List<Integer>> assignReplicasToBrokersRackAware(int
                                                                                int fixedStartIndex,
                                                                                int startPartitionId) {
        Map<Integer, String> brokerRackMap = new HashMap<>();
-        brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.get()));
+        brokerMetadatas.forEach(m -> brokerRackMap.put(m.id, m.rack.orElseThrow(() -> new AdminOperationException("Not all brokers have rack information for replica rack aware assignment."))));


Could you please add UT for it? AdminRackAwareTest is a good class to have the new test.

Could you please add UT for it? AdminRackAwareTest is a good class to have the new test.

I have added a new unit test for it. If there are any areas that could be improved, please provide me with some guidance and suggestions.

chia7712

LGTM

MINOR: Eliminate warnings for AdminUtils

aec8cde

Merge branch 'apache:trunk' into trunk

73bca12

LoganZhuZzz and others added 2 commits June 27, 2024 18:40

MINOR: Fix build issues

b2e3714

Merge branch 'apache:trunk' into trunk

c50d997

gongxuanzhang reviewed Jun 27, 2024

View reviewed changes

MINOR: Original code is straightforward enough, revert and refactor.

6a3b3d5

chia7712 reviewed Jun 27, 2024

View reviewed changes

LoganZhuZzz added 2 commits June 28, 2024 09:39

Merge branch 'apache:trunk' into trunk

2d06142

Merge branch 'apache:trunk' into trunk

1ad9d29

gongxuanzhang reviewed Jun 28, 2024

View reviewed changes

MINOR: Revert unrelated changes

87e1734

LoganZhuZzz force-pushed the trunk branch from 87fa82e to 87e1734 Compare June 28, 2024 10:33

LoganZhuZzz added 2 commits June 29, 2024 07:19

Merge branch 'apache:trunk' into trunk

1e35470

Merge branch 'apache:trunk' into trunk

6e36429

chia7712 reviewed Jun 29, 2024

View reviewed changes

LoganZhuZzz and others added 2 commits June 30, 2024 12:12

Merge branch 'apache:trunk' into trunk

eaf9a5e

MINOR: Add unit test for AdminUtils

e2ff23c

chia7712 approved these changes Jun 30, 2024

View reviewed changes

chia7712 merged commit 33f5995 into apache:trunk Jun 30, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Eliminate warnings for AdminUtils #16470

MINOR: Eliminate warnings for AdminUtils #16470

LoganZhuZzz commented Jun 27, 2024

gongxuanzhang commented Jun 27, 2024

LoganZhuZzz commented Jun 27, 2024

gongxuanzhang Jun 27, 2024

gongxuanzhang commented Jun 27, 2024

chia7712 left a comment

chia7712 Jun 27, 2024

LoganZhuZzz Jun 28, 2024

chia7712 Jun 28, 2024

LoganZhuZzz Jun 28, 2024

gongxuanzhang Jun 28, 2024

gongxuanzhang Jun 28, 2024

gongxuanzhang commented Jun 28, 2024

LoganZhuZzz commented Jun 28, 2024

chia7712 left a comment

chia7712 Jun 29, 2024

LoganZhuZzz Jun 30, 2024

chia7712 left a comment

MINOR: Eliminate warnings for AdminUtils #16470

MINOR: Eliminate warnings for AdminUtils #16470

Conversation

LoganZhuZzz commented Jun 27, 2024

gongxuanzhang commented Jun 27, 2024

LoganZhuZzz commented Jun 27, 2024

Choose a reason for hiding this comment

gongxuanzhang commented Jun 27, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongxuanzhang commented Jun 28, 2024

LoganZhuZzz commented Jun 28, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment