Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-14253 - More informative logging #13253

Merged
merged 2 commits into from Feb 17, 2023
Merged

KAFKA-14253 - More informative logging #13253

merged 2 commits into from Feb 17, 2023

Conversation

philipnee
Copy link
Collaborator

Includes 2 requirements from the ticket:

  • Include the number of members in the group (I.e., "15 members participating" and "to 15 clients as")
  • sort the member ids (to help compare the membership and assignment across rebalances)

Committer Checklist (excluded from commit message)

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

@philipnee philipnee marked this pull request as ready for review February 15, 2023 22:11
@philipnee
Copy link
Collaborator Author

Ideally, we want "1 member" or "n members". But we don't need this much grammatical optimization.

guozhangwang
guozhangwang reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @philipnee . Just one nit, otherwise LGTM.

...ams/src/main/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignor.java Outdated
@@ -637,8 +638,12 @@ private boolean assignTasksToClients(final Cluster fullMetadata,
statefulTasks,
assignmentConfigs);

log.info("Assigned tasks {} including stateful {} to clients as: \n{}.",
allTasks, statefulTasks, clientStates.entrySet().stream()
log.info("{} assigned tasks {} including stateful {} to clients as: \n{}.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Assigned {} tasks including ... to {} clients as ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, acked.

@philipnee
Copy link
Collaborator Author

Failures seem unrelated

Build / JDK 11 and Scala 2.13 / testReplicationIsCreatingTopicsUsingProvidedForwardingAdmin() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest
2m 15s
Build / JDK 11 and Scala 2.13 / testReplicationIsCreatingTopicsUsingProvidedForwardingAdmin() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest
2m 22s
Build / JDK 8 and Scala 2.12 / testCreatePartitionsUseProvidedForwardingAdmin() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest
2m 20s
Build / JDK 8 and Scala 2.12 / testConnectorReconfiguration – org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest
33s
Build / JDK 8 and Scala 2.12 / [1] Type=ZK, Name=testRegisterZkBrokerInKraft, MetadataVersion=3.4-IV0, Security=PLAINTEXT – kafka.server.KafkaServerKRaftRegistrationTest
8s
Build / JDK 8 and Scala 2.12 / [1] Type=ZK, Name=testRestartOldIbpZkBrokerInMigrationMode, MetadataVersion=3.3-IV0, Security=PLAINTEXT – kafka.server.KafkaServerKRaftRegistrationTest
4s
Build / JDK 8 and Scala 2.12 / [1] false – org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest
7m 15s

@guozhangwang guozhangwang merged commit 82d5720 into apache:trunk Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guozhangwang guozhangwang guozhangwang left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@philipnee @guozhangwang