[ 
https://issues.apache.org/jira/browse/KAFKA-820?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13609124#comment-13609124
 ] 

Jun Rao commented on KAFKA-820:
-------------------------------

Thanks for patch v3. A few minor comments. Once addressed, the patch can be 
checked in.

30. AdminUtil.getBrokerInfoFromCache(): The following line uses double negation
    brokerMetadata.filterNot(!_.isDefined).map(_.get)
It will be easier to read if we do
    brokerMetadata.filter(_.isDefined).map(_.get)

31. AdminUtil.fetchTopicMetadataFromZk():
31.1 In the following statement, should the catch clause be moved to after the 
first two statements inside try? Otherwise, we will unnecessarily wrap a 
ReplicaNotAvailableException over another ReplicaNotAvailableException.
          try {
            replicaInfo = getBrokerInfoFromCache(zkClient, cachedBrokerInfo, 
replicas.map(id => id.toInt))
            isrInfo = getBrokerInfoFromCache(zkClient, cachedBrokerInfo, 
inSyncReplicas)
            if(replicaInfo.size < replicas.size)
              throw new ReplicaNotAvailableException("Replica information not 
available for following brokers: " +
                
replicas.filterNot(replicaInfo.map(_.id).contains(_)).mkString(","))
            if(isrInfo.size < inSyncReplicas.size)
              throw new ReplicaNotAvailableException("In Sync Replica 
information not available for following brokers: " +
                
inSyncReplicas.filterNot(isrInfo.map(_.id).contains(_)).mkString(","))
          } catch {
            case e => throw new ReplicaNotAvailableException(e)
          }
31.2 Similarly, in the following try/catch clause, should we move the try/catch 
clause inside case Some(l)? Otherwise, we will be wrapping a 
LeaderNotAvailableException over another LeaderNotAvailableException.
          try {
            leaderInfo = leader match {
              case Some(l) => Some(getBrokerInfoFromCache(zkClient, 
cachedBrokerInfo, List(l)).head)
              case None => throw new LeaderNotAvailableException("No leader 
exists for partition " + partition)
            }
          } catch {
            case e => throw new LeaderNotAvailableException("Leader not 
available for topic %s partition %d".format(topic, partition), e)
          }


                
> Topic metadata request handling fails to return all metadata about replicas
> ---------------------------------------------------------------------------
>
>                 Key: KAFKA-820
>                 URL: https://issues.apache.org/jira/browse/KAFKA-820
>             Project: Kafka
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 0.8
>            Reporter: Neha Narkhede
>            Assignee: Neha Narkhede
>            Priority: Blocker
>              Labels: kafka-0.8
>         Attachments: kafka-820-v1.patch, kafka-820-v2.patch, 
> kafka-820-v3.patch
>
>
> The admin utility that fetches topic metadata needs improvement in error 
> handling. While fetching replica and isr broker information, if one of the 
> replicas is offline, it fails to fetch the replica and isr info for the rest 
> of them. This creates confusion on the client since it seems to the client 
> the rest of the brokers are offline as well.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to