CalvinConfluent commented on code in PR #14706: URL: https://github.com/apache/kafka/pull/14706#discussion_r1384373047
########## metadata/src/main/java/org/apache/kafka/controller/BrokersToElrs.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.controller; Review Comment: To help review this file, you can refer to the BrokersToIsrs.java The reasons not to modify the BrokersToIsrs are: 1. The BrokersToIsrs is complicated, hard to extend. 2. You may think that we can simply let the BrokersToIsrs treat the ISR and ELR the same, storing the ISR and ELR together. However, the ISR and ELR do not behave exactly the same(fence a broker will only remove it from ISRs). ########## metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java: ########## @@ -466,6 +466,9 @@ private void maybeUpdateLastKnownLeader(PartitionChangeRecord record) { partition.lastKnownElr[0] != partition.leader)) { // Only update the last known leader when the first time the partition becomes leaderless. record.setLastKnownELR(Arrays.asList(partition.leader)); + } else if (record.leader() != NO_LEADER && record.leader() != NO_LEADER_CHANGE && partition.leader == NO_LEADER) { Review Comment: Refactor the last known leader handling a bit. ########## metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java: ########## @@ -490,6 +493,10 @@ private void maybeUpdateRecordElr(PartitionChangeRecord record) { targetLastKnownElr = Replicas.toList(partition.lastKnownElr); } + // If the last known ELR is expected to store the last known leader, the lastKnownElr field should be updated + // later in maybeUpdateLastKnownLeader. + if (useLastKnownLeaderInBalancedRecovery) return; Review Comment: Refactor the last known leader handling a bit. ########## metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java: ########## @@ -1294,8 +1315,11 @@ void handleBrokerFenced(int brokerId, List<ApiMessageAndVersion> records) { */ void handleBrokerUnregistered(int brokerId, long brokerEpoch, List<ApiMessageAndVersion> records) { - generateLeaderAndIsrUpdates("handleBrokerUnregistered", brokerId, NO_LEADER, records, - brokersToIsrs.partitionsWithBrokerInIsr(brokerId)); + generateLeaderAndIsrUpdates("handleBrokerUnregistered", brokerId, NO_LEADER, NO_LEADER, records, + new PartitionsOnReplicaIteratorChain(Arrays.asList( Review Comment: We can avoid using the iterator chain by calling generateLeaderAndIsrUpdates twice. I am open to both ways. -- 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
