KAFKA-8442:Inconsistent ISR output in topic command #6836

huxihx · 2019-05-29T06:18:42Z

https://issues.apache.org/jira/browse/KAFKA-8442

Modify leader and ISR output for --zookeeper path in TopicCommand, to be consistent with --bootstrap-server path.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

huxihx · 2019-05-29T12:15:33Z

retest this please

hachikuji · 2019-05-29T16:02:34Z

core/src/main/scala/kafka/admin/TopicCommand.scala

                  assignedReplicas,
-                  isr = if (leaderIsrEpoch.isEmpty) Seq.empty[Int] else leaderIsrEpoch.get.leaderAndIsr.isr,
+                  isr = if (leaderIsrEpoch.isEmpty) Seq.empty[Int] else leaderIsrEpoch.get.leaderAndIsr.isr.filter(liveBrokers.contains),


This is sort of the opposite of what I suggested in the JIRA. It is often still useful to know the ISR information even when the broker is offline. Maybe it would be better for now to list the isr as "not available" when using --bootstrap-server?

Thanks for the comments. Does it make sense to display a nonempty isr for a partition with no leader actually?
Topic: foo Partition: 0 Leader: -1 Replicas: 1,3 Isr: 1

You said it showed the current ISR correctly. I am very intrigued to know the reason. The output for --bootstrap-server, in my view, reflects the correct state for a partition.

The ISR is still valid if the leader is offline. It tells you which replica must be brought back online in order to avoid the need for unclean leader election.

hachikuji · 2019-05-29T16:07:57Z

core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala

+        LeaderAndIsr(leader = 0, leaderEpoch = 0, isr = List(0, 1), zkVersion = 0),
+        controllerEpoch = 0)
+    )
+    topicService.zkClient.createTopicPartitionStatesRaw(leaderIsrAndControllerEpochs, ZkVersion.MatchAnyVersion)


Rather than manually updating the leader and isr state, which may have unexpected behavior, wouldn't it be easier to set the replica assignment in the create topic command?

huxihx · 2019-05-30T07:55:45Z

retest this please

hachikuji · 2019-05-31T06:30:59Z

core/src/main/scala/kafka/admin/TopicCommand.scala

@@ -443,7 +443,7 @@ object TopicCommand extends Logging {
    print("\tPartition: " + tp.partition)
    print("\tLeader: " + (if(tp.leader.isDefined) tp.leader.get else "none"))
    print("\tReplicas: " + tp.assignedReplicas.mkString(","))
-    print("\tIsr: " + tp.isr.mkString(","))
+    print("\tIsr: " + (if(tp.isr.isEmpty) "not available" else tp.isr.mkString(",")))


The case we're trying to detect is here: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/MetadataCache.scala#L93. Basically when there is no current leader, the broker will return an empty ISR. I think the logic here should match that. So if there is no leader defined, then we can represent the ISR as "not available" or maybe just "N/A". Otherwise, we can print the ISR as it is. Does that make sense?

huxihx · 2019-06-04T10:53:04Z

retest this please

hachikuji · 2019-06-12T18:50:46Z

core/src/main/scala/kafka/admin/TopicCommand.scala

@@ -443,7 +443,7 @@ object TopicCommand extends Logging {
    print("\tPartition: " + tp.partition)
    print("\tLeader: " + (if(tp.leader.isDefined) tp.leader.get else "none"))
    print("\tReplicas: " + tp.assignedReplicas.mkString(","))
-    print("\tIsr: " + tp.isr.mkString(","))
+    print("\tIsr: " + (if(tp.leader.isDefined) tp.isr.mkString(",") else "none"))


We still have two problems:

This affects both the output using --zookeeper and --bootstrap-server. As I said in the first comment, it is still useful to display the ISR when the leader is offline if we have it available. The problem here is that we do not have an accurate ISR when --bootstrap-server is used.

The term "none" may cause confusion. We are trying to convey the fact that we just don't know what the current ISR is.

Maybe we should try to approach this from a different perspective. The root of the problem is that the Metadata API does not return the ISR faithfully when there is no leader. Can we change that? The rest of the information is returned accurately including the replica set and the offline replicas. I cannot think of a good reason for the inconsistency. I'd suggest we change the behavior here: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/MetadataCache.scala#L98. Then we do not need to handle the case specially here.

…strap-server https://issues.apache.org/jira/browse/KAFKA-8442 Modify leader and ISR output for --zookeeper path in TopicCommand, to be consistent with --bootstrap-server path.

huxihx · 2019-07-01T01:04:51Z

retest this please

hachikuji

Thanks, left a couple more comments.

hachikuji · 2019-07-01T21:56:44Z

core/src/main/scala/kafka/admin/TopicCommand.scala

@@ -120,7 +120,10 @@ object TopicCommand extends Logging {
      opts.reportUnavailablePartitions && hasUnavailablePartitions(partitionDescription)
    }
    private def hasUnderMinIsrPartitions(partitionDescription: PartitionDescription) = {
-      partitionDescription.isr.size < partitionDescription.minIsrCount
+      if (partitionDescription.leader.isDefined)


Hmm.. I'm not sure I understand the reason for this change. Can you explain?

TopicCommandWithAdminClientTest.testDescribeUnderMinIsrPartitionsMixed will fail. If leader is not available, the partition must be a under-ISR one.

Hmm, good point. Since min.insync.replicas cannot be 0, I guess we could simplify this.

partitionDescription.leader.isEmpty || partitionDescription.isr.size < partitionDescription.minIsrCount

hachikuji · 2019-07-01T22:00:43Z

core/src/main/scala/kafka/server/MetadataCache.scala

@@ -91,13 +93,10 @@ class MetadataCache(brokerId: Int) extends Logging {
              if (errorUnavailableListeners) Errors.LISTENER_NOT_FOUND else Errors.LEADER_NOT_AVAILABLE
            }
            new MetadataResponse.PartitionMetadata(error, partitionId.toInt, Node.noNode(),
-              Optional.empty(), replicaInfo.asJava, java.util.Collections.emptyList(),
+              Optional.empty(), replicaInfo.asJava, isrInfo.asJava,


Can we have a test case for this change?

Do you think MetadataCacheTest#getTopicMetadataPartitionLeaderNotAvailable cover this?

hachikuji

LGTM

hachikuji · 2019-07-30T00:39:24Z

retest this please

hachikuji reviewed May 29, 2019

View reviewed changes

hachikuji reviewed May 31, 2019

View reviewed changes

huxihx force-pushed the KAFKA-8442 branch from 09839fc to a23f3cb Compare June 4, 2019 03:26

hachikuji reviewed Jun 12, 2019

View reviewed changes

huxihx added 4 commits June 27, 2019 14:19

KAFKA-8442:Inconsistent ISR output in topic command when using --boot…

2416959

…strap-server https://issues.apache.org/jira/browse/KAFKA-8442 Modify leader and ISR output for --zookeeper path in TopicCommand, to be consistent with --bootstrap-server path.

add_not_available_tag_for_empty_isr

5b7a3e2

Display none for ISR without leader

b9f3184

addressed Jason's comments

654949d

huxihx force-pushed the KAFKA-8442 branch from a23f3cb to 654949d Compare June 28, 2019 05:30

hachikuji reviewed Jul 1, 2019

View reviewed changes

hachikuji mentioned this pull request Jul 17, 2019

KAFKA-8453: AdminClient describeTopic should handle partition level errors #7089

Open

3 tasks

hachikuji approved these changes Jul 30, 2019

View reviewed changes

hachikuji merged commit c6286b2 into apache:trunk Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-8442:Inconsistent ISR output in topic command #6836

KAFKA-8442:Inconsistent ISR output in topic command #6836

huxihx commented May 29, 2019

huxihx commented May 29, 2019

hachikuji May 29, 2019

huxihx May 30, 2019

hachikuji May 30, 2019

hachikuji May 29, 2019

huxihx commented May 30, 2019

hachikuji May 31, 2019

huxihx commented Jun 4, 2019

hachikuji Jun 12, 2019

huxihx commented Jul 1, 2019

hachikuji left a comment

hachikuji Jul 1, 2019

huxihx Jul 17, 2019

hachikuji Jul 17, 2019

hachikuji Jul 1, 2019

huxihx Jul 19, 2019

hachikuji left a comment

hachikuji commented Jul 30, 2019

KAFKA-8442:Inconsistent ISR output in topic command #6836

KAFKA-8442:Inconsistent ISR output in topic command #6836

Conversation

huxihx commented May 29, 2019

Committer Checklist (excluded from commit message)

huxihx commented May 29, 2019

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

huxihx commented May 30, 2019

Choose a reason for hiding this comment

huxihx commented Jun 4, 2019

Choose a reason for hiding this comment

huxihx commented Jul 1, 2019

hachikuji 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

hachikuji left a comment

Choose a reason for hiding this comment

hachikuji commented Jul 30, 2019