You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/22 09:05:20 UTC
[GitHub] [kafka] showuon opened a new pull request #10172: MINOR: add toString to Subscription classes
showuon opened a new pull request #10172:
URL: https://github.com/apache/kafka/pull/10172
We now have the debug log [here](https://github.com/apache/kafka/blob/ea8ae976504e7a3f5c6f4a7efa5069d03316b093/clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java#L587):
```java
log.debug("Performing assignment using strategy {} with subscriptions {}", assignor.name(), subscriptions);
```
But we didn't override the toString method for subscription class, so user will get the useless debug info:
```
Performing assignment using strategy cooperative-sticky with subscriptions {consumer-groupId-1-7fd39d50-5dc1-46f0-860e-f0361eb2afc0=org.apache.kafka.clients.consumer.ConsumerPartitionAssignor$Subscription@247d8ae,
consumer-groupId-1-09ffd69e-ce6c-4212-9d2c-218d60ad8d19=org.apache.kafka.clients.consumer.ConsumerPartitionAssignor$Subscription@48974e45}
```
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#discussion_r580802728
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java
##########
@@ -132,6 +132,16 @@ public void setGroupInstanceId(Optional<String> groupInstanceId) {
public Optional<String> groupInstanceId() {
return groupInstanceId;
}
+
+ @Override
+ public String toString() {
+ return "Subscription(" +
+ "topics=" + topics +
+ ", userData=" + userData +
Review comment:
Oh, I didn't notice it! I logged the remaining size of the userData like the `Assignment.toString()` did. Thank you.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#discussion_r580767910
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java
##########
@@ -132,6 +132,16 @@ public void setGroupInstanceId(Optional<String> groupInstanceId) {
public Optional<String> groupInstanceId() {
return groupInstanceId;
}
+
+ @Override
+ public String toString() {
+ return "Subscription(" +
+ "topics=" + topics +
+ ", userData=" + userData +
+ ", ownedPartitions=" + ownedPartitions +
+ (groupInstanceId.isPresent() ? ", groupInstanceId=" + groupInstanceId.get() : "") +
Review comment:
How about using lambda? `(groupInstanceId.map(s -> ", groupInstanceId=" + s).orElse(""))`
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java
##########
@@ -132,6 +132,16 @@ public void setGroupInstanceId(Optional<String> groupInstanceId) {
public Optional<String> groupInstanceId() {
return groupInstanceId;
}
+
+ @Override
+ public String toString() {
+ return "Subscription(" +
+ "topics=" + topics +
+ ", userData=" + userData +
Review comment:
The type of `userData` is `ByteBuffer` so it makes no sense to print it by `toString`. Maybe we can print the size?
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java
##########
@@ -132,6 +132,16 @@ public void setGroupInstanceId(Optional<String> groupInstanceId) {
public Optional<String> groupInstanceId() {
return groupInstanceId;
}
+
+ @Override
+ public String toString() {
+ return "Subscription(" +
+ "topics=" + topics +
+ ", userData=" + userData +
+ ", ownedPartitions=" + ownedPartitions +
+ (groupInstanceId.isPresent() ? ", groupInstanceId=" + groupInstanceId.get() : "") +
Review comment:
For another, it seems to me "null" is more suitable than empty string.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#issuecomment-784032562
```
kafka.server.ListOffsetsRequestTest.testResponseIncludesLeaderEpoch()
org.opentest4j.AssertionFailedError: expected: <(0,0)> but was: <(-1,-1)>
```
unrelated error
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#issuecomment-783216836
@hachikuji , could you help review this simple PR? Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10172:
URL: https://github.com/apache/kafka/pull/10172
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#issuecomment-783216836
@hachikuji , could you help review this PR? Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#issuecomment-783954552
@chia7712 , thanks for the comments. Please take a look again. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10172: MINOR: add toString to Subscription classes
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10172:
URL: https://github.com/apache/kafka/pull/10172#discussion_r580802108
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java
##########
@@ -132,6 +132,16 @@ public void setGroupInstanceId(Optional<String> groupInstanceId) {
public Optional<String> groupInstanceId() {
return groupInstanceId;
}
+
+ @Override
+ public String toString() {
+ return "Subscription(" +
+ "topics=" + topics +
+ ", userData=" + userData +
+ ", ownedPartitions=" + ownedPartitions +
+ (groupInstanceId.isPresent() ? ", groupInstanceId=" + groupInstanceId.get() : "") +
Review comment:
Updated. Thank you!
----------------------------------------------------------------
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:
users@infra.apache.org