KAFKA-7236: Add --under-min-isr option to describe topics command (KIP-351) #6224
Conversation
|
@viktorsomogyi @gwenshap can you give this a review? Thanks~ |
| @@ -99,29 +99,37 @@ object TopicCommand extends Logging { | |||
| leader: Option[Int], | |||
| assignedReplicas: Seq[Int], | |||
| isr: Seq[Int], | |||
| minIsr: Int, | |||
There was a problem hiding this comment.
Since this is just a count, I suggest using minIsrCount to avoid confusion with isr which is also a list of Int.
| @@ -220,13 +228,16 @@ object TopicCommand extends Logging { | |||
| } | |||
| } | |||
| if (describeOptions.describePartitions) { | |||
| val computedMinIsr = if (opts.reportUnderMinIsrPartitions) | |||
There was a problem hiding this comment.
Similarly here: configuredMinIsrCount?
core/src/test/scala/unit/kafka/admin/TopicCommandWithAdminClientTest.scala
Show resolved
Hide resolved
| @@ -536,6 +548,8 @@ object TopicCommand extends Logging { | |||
| "if set when describing topics, only show under replicated partitions") | |||
| private val reportUnavailablePartitionsOpt = parser.accepts("unavailable-partitions", | |||
| "if set when describing topics, only show partitions whose leader is not available") | |||
| private val reportUnderMinIsrPartitionsOpt = parser.accepts("under-minisr-partitions", | |||
There was a problem hiding this comment.
In the KIP the config is proposed as under-min-isr-partitions. Is there a reason you used a different name? I think the name in the KIP (with a dash between min and isr) looks and reads better.
There was a problem hiding this comment.
Good catch, thanks!
|
Hi @vahidhashemian, thanks for the review! I have added a commit addressing your comments. I understand that the feature freeze for |
There was a problem hiding this comment.
Thanks for updating the PR. I left a couple of minor nits that you can hopefully fix quickly. Then, I'll approve the PR, and merge once the trunk version is bumped.
Unfortunately, since the cutoff for 2.2 is passed this will be merged to trunk for the following release.
| @@ -536,13 +536,78 @@ class TopicCommandWithAdminClientTest extends KafkaServerTestHarness with Loggin | |||
| killBroker(0) | |||
| val output = TestUtils.grabConsoleOutput( | |||
| topicService.describeTopic(new TopicCommandOptions(Array("--under-replicated-partitions")))) | |||
| println(output) | |||
There was a problem hiding this comment.
nit: Unintentional println here?
| * (4) topic with fully replicated partition | ||
| * | ||
| * Output should only display the (1) topic with partition under min ISR count and (3) topic with offline partition | ||
| */ |
There was a problem hiding this comment.
nit: Please change references to three/3 to four/4 since there are 4 topics now.
|
retest this please |
1 similar comment
|
retest this please |
|
Hi @vahidhashemian, thanks for the merge. I see that the trunk version was bumped up after this PR was merged. Did we squeeze this into |
|
@KevinLiLu I believe this KIP was originally included in 2.2.0 release plan. So this PR should also be included since the feature freeze is this Friday. @mjsax could you please confirm? Thanks! |
|
KIPs must be merged before feature freeze deadline (cf. https://cwiki.apache.org/confluence/display/KAFKA/Time+Based+Release+Plan) that was two week ago. Thus, this KIP slips for 2.2. Sorry. It's already marked as postponed to 2.3 in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100827512 |
|
@mjsax no worries, thanks for the clarification! |
|
Thanks for clarifying @mjsax. |
…P-351) (apache#6224) * KAFKA-7236: Add --under-min-isr option to describe topics command (KIP-351) * Minor changes to description and make test consistent with others * Fix option, and add additional test with mixed partition status * Add fully-replicated-topic to test case * Address review nits
KIP-351
--under-min-isroption toTopicCommandto report partitions under the configuredmin.insync.replicasvalueCommitter Checklist (excluded from commit message)