subject:"\[GitHub\] \[kafka\] VinceMu commented on a change in pull request #8666\: KAFKA\-9479\: Describe consumer group \-\-state \-\-all\-groups show header once"

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435218619



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"
-  val coordinatorColLen = Math.max(25, coordinator.length)
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format(state.group, coordinator, state.assignmentStrategy, state.state, 
state.numMembers))
-  println()
+  
(state.group,coordinator,state.assignmentStrategy,state.state,state.numMembers)
 }
+  val hasAllGroups = opts.options.has(opts.allGroupsOpt)
+  if(stateProps.nonEmpty && hasAllGroups){
+val headerLengthOffset = 
Math.max(25,stateProps.maxBy{_._2.length}._2.length)
+print(s"\n%${-headerLengthOffset}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))

Review comment:
   `Print`s with explicit '\n' are used throughout ConsumerGroupCommand. My 
guess as to why this is because `println` uses `System.lineSeparator` which may 
not always be '\n' depending on the operating system. Switching to `println` 
would also involve modifying the tests which count lines by splitting on `\n'. 
Because of these reasons I'm hesitant to switch over to `println`. Thoughts?
   
   The idea of using the coordinator length as an offset to format the length 
existed before this PR. The change I've now implemented is instead of 
calculating offset length per group, we calculate it only once by taking the 
largest possible offset from all groups. 

##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"
-  val coordinatorColLen = Math.max(25, coordinator.length)
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format(state.group, coordinator, state.assignmentStrategy, state.state, 
state.numMembers))
-  println()
+  
(state.group,coordinator,state.assignmentStrategy,state.state,state.numMembers)
 }
+  val hasAllGroups = opts.options.has(opts.allGroupsOpt)
+  if(stateProps.nonEmpty && hasAllGroups){
+val headerLengthOffset = 
Math.max(25,stateProps.maxBy{_._2.length}._2.length)
+print(s"\n%${-headerLengthOffset}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))

Review comment:
   `Print`s with explicit '\n' are used throughout ConsumerGroupCommand. My 
guess as to why this is because `println` uses `System.lineSeparator` which may 
not always be '\n' depending on the operating system. Switching to `println` 
would also involve modifying the tests which count lines by splitting on '\n'. 
Because of these reasons I'm hesitant to switch over to `println`. Thoughts?
   
   The idea of using the coordinator length as an offset to format the length 
existed before this PR. The change I've now implemented is instead of 
calculating offset length per group, we calculate it only once by taking the 
largest possible offset from all groups. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435213295



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"
-  val coordinatorColLen = Math.max(25, coordinator.length)
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format(state.group, coordinator, state.assignmentStrategy, state.state, 
state.numMembers))
-  println()
+  
(state.group,coordinator,state.assignmentStrategy,state.state,state.numMembers)
 }
+  val hasAllGroups = opts.options.has(opts.allGroupsOpt)
+  if(stateProps.nonEmpty && hasAllGroups){
+val headerLengthOffset = 
Math.max(25,stateProps.maxBy{_._2.length}._2.length)
+print(s"\n%${-headerLengthOffset}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
+  }
+  stateProps.foreach{ 
case(group,coordinator,assignmentStrategy,state,numMembers)=>
+val offset = -Math.max(25,coordinator.length)

Review comment:
   Thanks for that find, I've now implemented offset computation only once. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435213295



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"
-  val coordinatorColLen = Math.max(25, coordinator.length)
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format(state.group, coordinator, state.assignmentStrategy, state.state, 
state.numMembers))
-  println()
+  
(state.group,coordinator,state.assignmentStrategy,state.state,state.numMembers)
 }
+  val hasAllGroups = opts.options.has(opts.allGroupsOpt)
+  if(stateProps.nonEmpty && hasAllGroups){
+val headerLengthOffset = 
Math.max(25,stateProps.maxBy{_._2.length}._2.length)
+print(s"\n%${-headerLengthOffset}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
+  }
+  stateProps.foreach{ 
case(group,coordinator,assignmentStrategy,state,numMembers)=>
+val offset = -Math.max(25,coordinator.length)

Review comment:
   Done this :) 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435212607



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"
-  val coordinatorColLen = Math.max(25, coordinator.length)
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format("GROUP", "COORDINATOR (ID)", "ASSIGNMENT-STRATEGY", "STATE", 
"#MEMBERS"))
-  print(s"\n%${-coordinatorColLen}s %-25s %-20s %-15s 
%s".format(state.group, coordinator, state.assignmentStrategy, state.state, 
state.numMembers))
-  println()
+  
(state.group,coordinator,state.assignmentStrategy,state.state,state.numMembers)
 }
+  val hasAllGroups = opts.options.has(opts.allGroupsOpt)
+  if(stateProps.nonEmpty && hasAllGroups){

Review comment:
   KAFKA-9479 entails only printing the header once when the `all-groups` 
flag is present. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435211987



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}
+.map{case (_,state)=>
   val coordinator = 
s"${state.coordinator.host}:${state.coordinator.port} 
(${state.coordinator.idString})"

Review comment:
   Thanks for the suggestion, I've extracted this into the `GroupState` 
case class. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

2020-06-04 Thread GitBox



VinceMu commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r435211667



##
File path: core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala
##
@@ -291,14 +291,21 @@ object ConsumerGroupCommand extends Logging {
 }
 
 private def printStates(states: Map[String, GroupState]): Unit = {
-  for ((groupId, state) <- states) {
-if (shouldPrintMemberState(groupId, Some(state.state), Some(1))) {
+  val stateProps =  
states.filter{case(groupId,state)=>shouldPrintMemberState(groupId, 
Some(state.state), Some(1))}

Review comment:
   I wanted to avoid changing any existing behaviour with this PR. In light 
of this, could we print all the groups regardless of their state when the 
`--all-groups` flag is given? 
   





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

[GitHub] [kafka] VinceMu commented on a change in pull request #8666: KAFKA-9479: Describe consumer group --state --all-groups show header once

6 matches

Site Navigation

Mail list logo

Footer information