[GitHub] [kafka] ning2008wisc commented on pull request #9224: KAFKA-10304: refactor MM2 integration tests
ning2008wisc commented on pull request #9224: URL: https://github.com/apache/kafka/pull/9224#issuecomment-731934229 @mimaison Thanks again for your previous detailed review. I updated the PR to resolve the exact 2 concerns you raised. Very appreciated for your another review! 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] chia7712 merged pull request #9637: MONOR; Wrong command line suggestion
chia7712 merged pull request #9637: URL: https://github.com/apache/kafka/pull/9637 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
[jira] [Commented] (KAFKA-10753) check if ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG > 0
[ https://issues.apache.org/jira/browse/KAFKA-10753?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17237054#comment-17237054 ] lqjacklee commented on KAFKA-10753: --- [~dengziming] should we create another jira to track the issue? > check if ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG > 0 > --- > > Key: KAFKA-10753 > URL: https://issues.apache.org/jira/browse/KAFKA-10753 > Project: Kafka > Issue Type: Improvement > Components: consumer >Reporter: shiqihao >Assignee: lqjacklee >Priority: Minor > > I accidentally set ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG = 0, CPU > running at 100%. > Could we add a check if ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG > 0 > while start consumer? -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [kafka] mjsax merged pull request #9618: MINOR: change default TX timeout only if EOS is enabled
mjsax merged pull request #9618: URL: https://github.com/apache/kafka/pull/9618 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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528407594
##
File path: clients/src/main/resources/common/message/MetadataRequest.json
##
@@ -31,9 +31,11 @@
// Starting in version 8, authorized operations can be requested for
cluster and topic resource.
//
// Version 9 is the first flexible version.
+// Version 10 add topicId
{ "name": "Topics", "type": "[]MetadataRequestTopic", "versions": "0+",
"nullableVersions": "1+",
"about": "The topics to fetch metadata for.", "fields": [
- { "name": "Name", "type": "string", "versions": "0+", "entityType":
"topicName",
+ { "name": "TopicId", "type": "uuid", "versions": "10+", "ignorable":
true, "about": "The topic id." },
+ { "name": "Name", "type": "string", "versions": "0+", "entityType":
"topicName", "ignorable": true,
Review comment:
As discussed in the mailing thread and the KIP writeup, this should be
"nullable" rather than ignorable
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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528407594
##
File path: clients/src/main/resources/common/message/MetadataRequest.json
##
@@ -31,9 +31,11 @@
// Starting in version 8, authorized operations can be requested for
cluster and topic resource.
//
// Version 9 is the first flexible version.
+// Version 10 add topicId
{ "name": "Topics", "type": "[]MetadataRequestTopic", "versions": "0+",
"nullableVersions": "1+",
"about": "The topics to fetch metadata for.", "fields": [
- { "name": "Name", "type": "string", "versions": "0+", "entityType":
"topicName",
+ { "name": "TopicId", "type": "uuid", "versions": "10+", "ignorable":
true, "about": "The topic id." },
+ { "name": "Name", "type": "string", "versions": "0+", "entityType":
"topicName", "ignorable": true,
Review comment:
As discussed in the mailing thread and the KIP writeup, this should be
"nullable"
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] jolshan commented on pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on pull request #9622: URL: https://github.com/apache/kafka/pull/9622#issuecomment-731847801 @dengziming Looks pretty good so far to me! I think it would be useful to write a few unit/integration tests to ensure the metadata snapshot behavior and describe topics work as expected. 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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528404378
##
File path: core/src/main/scala/kafka/api/ApiVersion.scala
##
@@ -424,6 +426,13 @@ case object KAFKA_2_7_IV2 extends DefaultApiVersion {
val id: Int = 30
}
+case object KAFKA_2_7_IV3 extends DefaultApiVersion {
Review comment:
nit: this should be KAFKA_2_8_IV0, or KAFKA_2_8_IV1 if
https://github.com/apache/kafka/pull/9601 merges
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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528401568
##
File path: core/src/main/scala/kafka/admin/TopicCommand.scala
##
@@ -113,6 +114,7 @@ object TopicCommand extends Logging {
def printDescription(): Unit = {
val configsAsString = config.entries.asScala.filter(!_.isDefault).map {
ce => s"${ce.name}=${ce.value}" }.mkString(",")
print(s"Topic: $topic")
+ print(s"\tTopicId: ${if(topicId == Uuid.ZERO_UUID) "" else topicId}")
Review comment:
Do we want to print an empty string for TopicId? Would it make sense to
include the Zero Uuid or not print a topic ID label at all?
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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528401568
##
File path: core/src/main/scala/kafka/admin/TopicCommand.scala
##
@@ -113,6 +114,7 @@ object TopicCommand extends Logging {
def printDescription(): Unit = {
val configsAsString = config.entries.asScala.filter(!_.isDefault).map {
ce => s"${ce.name}=${ce.value}" }.mkString(",")
print(s"Topic: $topic")
+ print(s"\tTopicId: ${if(topicId == Uuid.ZERO_UUID) "" else topicId}")
Review comment:
Do we want to have a field TopicId that is empty? Would it make sense to
include the Zero Uuid or not print a topic ID label at all?
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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528401033
##
File path: clients/src/main/java/org/apache/kafka/common/Cluster.java
##
@@ -327,6 +350,18 @@ public Node controller() {
return controller;
}
+public Collection topicIds() {
+return topicIds.values();
+}
+
+public Uuid getTopicId(String topic) {
+return topicIds.getOrDefault(topic, Uuid.ZERO_UUID);
+}
+
+public String getTopicName(Uuid topiId) {
Review comment:
Also, for getTopicId, we do getOrDefault. Is there a reason we wouldn't
do that here?
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] jolshan commented on a change in pull request #9622: KAFKA-10547; add topicId in MetadataResp
jolshan commented on a change in pull request #9622:
URL: https://github.com/apache/kafka/pull/9622#discussion_r528366073
##
File path: clients/src/main/java/org/apache/kafka/common/Cluster.java
##
@@ -327,6 +350,18 @@ public Node controller() {
return controller;
}
+public Collection topicIds() {
+return topicIds.values();
+}
+
+public Uuid getTopicId(String topic) {
+return topicIds.getOrDefault(topic, Uuid.ZERO_UUID);
+}
+
+public String getTopicName(Uuid topiId) {
Review comment:
nit: `Uuid topicId`
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
[jira] [Assigned] (KAFKA-10725) Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest
[ https://issues.apache.org/jira/browse/KAFKA-10725?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Liju reassigned KAFKA-10725: Assignee: Liju > Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest > - > > Key: KAFKA-10725 > URL: https://issues.apache.org/jira/browse/KAFKA-10725 > Project: Kafka > Issue Type: Improvement > Components: streams, unit tests >Reporter: Guozhang Wang >Assignee: Liju >Priority: Minor > > These two integration tests are covering different issues, and have had their > own flakiness in the past. I think it's better to merge them into a single > class (and some of them can better be reduced to unit tests?) so that if > there's still flakiness, we only need to fix it in one place. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Assigned] (KAFKA-10725) Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest
[ https://issues.apache.org/jira/browse/KAFKA-10725?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Liju reassigned KAFKA-10725: Assignee: (was: Liju) > Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest > - > > Key: KAFKA-10725 > URL: https://issues.apache.org/jira/browse/KAFKA-10725 > Project: Kafka > Issue Type: Improvement > Components: streams, unit tests >Reporter: Guozhang Wang >Priority: Minor > > These two integration tests are covering different issues, and have had their > own flakiness in the past. I think it's better to merge them into a single > class (and some of them can better be reduced to unit tests?) so that if > there's still flakiness, we only need to fix it in one place. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [kafka] highluck commented on pull request #9640: KAFKA-10283; Consolidate client-level and consumer-level assignment within ClientState
highluck commented on pull request #9640: URL: https://github.com/apache/kafka/pull/9640#issuecomment-731766072 @guozhangwang Please review 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: us...@infra.apache.org
[GitHub] [kafka] highluck opened a new pull request #9640: KAFKA-10283; Consolidate client-level and consumer-level assignment within ClientState
highluck opened a new pull request #9640: URL: https://github.com/apache/kafka/pull/9640 KAFKA-10283 Consolidate client-level and consumer-level assignment within ClientState https://issues.apache.org/jira/browse/KAFKA-10283 ### 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: us...@infra.apache.org
[jira] [Assigned] (KAFKA-10725) Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest
[ https://issues.apache.org/jira/browse/KAFKA-10725?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Liju reassigned KAFKA-10725: Assignee: Liju > Merge StoreQueryIntegrationTest and QueryableStateIntegrationTest > - > > Key: KAFKA-10725 > URL: https://issues.apache.org/jira/browse/KAFKA-10725 > Project: Kafka > Issue Type: Improvement > Components: streams, unit tests >Reporter: Guozhang Wang >Assignee: Liju >Priority: Minor > > These two integration tests are covering different issues, and have had their > own flakiness in the past. I think it's better to merge them into a single > class (and some of them can better be reduced to unit tests?) so that if > there's still flakiness, we only need to fix it in one place. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [kafka] dengziming opened a new pull request #9639: KAFKA-10677; Complete fetches in purgatory immediately after resigning
dengziming opened a new pull request #9639: URL: https://github.com/apache/kafka/pull/9639 *More detailed description of your change* If the condition of fetch is satisfied, `BROKER_NOT_AVAILABLE` or `NOT_LEADER_OR_FOLLOWER` is returned when the leader is shutting down. so we just return `BROKER_NOT_AVAILABLE` with a message. *Summary of testing strategy* A simple unit test to verify fetches in purgatory is completed after resigning. ### 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: us...@infra.apache.org
[jira] [Assigned] (KAFKA-10677) Complete fetches in purgatory immediately after raft leader resigns
[ https://issues.apache.org/jira/browse/KAFKA-10677?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] dengziming reassigned KAFKA-10677: -- Assignee: dengziming > Complete fetches in purgatory immediately after raft leader resigns > --- > > Key: KAFKA-10677 > URL: https://issues.apache.org/jira/browse/KAFKA-10677 > Project: Kafka > Issue Type: Sub-task >Reporter: Jason Gustafson >Assignee: dengziming >Priority: Major > > The current logic does not complete fetches in purgatory immediately after > the leader has resigned. The idea was that there was no point in doing so > until the election had completed because clients would just have to retry. > However, the fetches in purgatory might correspond to requests from other > voters, so the concern is that this might delay a leader election. For > example, the voter might be trying to send a Vote request on the same socket > that is blocking on a pending Fetch. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [kafka] dengziming commented on pull request #9638: MINOR; Remove duplicate update updateLeaderEndOffsetAndTimestamp
dengziming commented on pull request #9638: URL: https://github.com/apache/kafka/pull/9638#issuecomment-731720762 ping @hachikuji to have a look. 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] dengziming opened a new pull request #9638: MINOR; Remove duplicate update updateLeaderEndOffsetAndTimestamp
dengziming opened a new pull request #9638: URL: https://github.com/apache/kafka/pull/9638 The `KafkaRaftClient.onBecomeLeader` will invoke `appendLeaderChangeMessage`, the call stack are: ``` 1. appendLeaderChangeMessage 1.1 flushLeaderLog 1.1.1 updateLeaderEndOffsetAndTimestamp 1.1.2 log.flush() ``` so the `updateLeoAndTs ` is already invoked, and `updateLeoAndTs` should only be invoked after leo change(or time change), since `log.flush()` will not change leo(and ts), it's unnessary to invoke twice. 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
