dajac commented on a change in pull request #8666:
URL: https://github.com/apache/kafka/pull/8666#discussion_r434554562
##########
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:
I suggest to extract this as a method `coordinatorString` in the
`GroupState` case class or in an inner function.
##########
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:
The computation of the offset is not consistent here. For all groups, it
will result in misaligning the group information. We should compute it once and
reuse it all the time.
##########
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:
What's the reason why you distinguish the all groups case here? It seems
that it was not case before.
##########
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:
We could use `println` and get ride of the `\n` here. Also, it seems
that `headerLengthOffset` is computed based on the coordinator string but is
used to format the group.
##########
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 wonder if we shouldn't simply get rid of `shouldPrintMemberState` and
print out all the groups in the table. `shouldPrintMemberState` seems to log
some info based on the state of the group. I think that the state would be self
explanatory in the table. Would this make sense?
----------------------------------------------------------------
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
