Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
mjsax commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1520482326 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: Did a follow up PR: https://github.com/apache/kafka/pull/15519 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
dajac commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1513841644 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: Right. This part is clearly wrong in the doc. It cannot change within the lifetime of the consumer. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
mjsax commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1509341591 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: I think "setting the id all the time" vs "omitting it" is an important but orthogonal question. -- The comment say that it must be set "if has changed", but it should never change, right? > So the comment in the schema was wrong Is this about setting the id vs not setting it, or about the original question if it could change? -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
dajac commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1509278562 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: The current implementation actually requires the static member id to be set all the time if the consumer uses the static membership. So the comment in the schema was wrong. I need to go back to the implementation to see whether we could relax it and only require it in the first request, when joining. I will check and let you guys know. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
lucasbru merged PR #15419: URL: https://github.com/apache/kafka/pull/15419 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
cadonna commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1500644733 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: I totally agree. I just mentioned it to start a discussion not to block this PR or request immediate changes in the group coordinator. ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: I totally agree. I just mentioned it to start a discussion, not to block this PR or request immediate changes in the group coordinator. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
lucasbru commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1500632283 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: I was wondering that as well. I wanted to get this out of the way to stabilize the tests. It's not wrong for the client to send the instance ID here. If we are worried, we can wait for David to come back from PTO. I wouldn't want to just relax the requirements in the coordinator on my own. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
cadonna commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1500548287 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: Might the fact that an group instance ID is required when leaving the group even be a mistake in the group coordinator? As far as I can see, the member epoch that is sent already contains the information that a static member leaves the group and the group coordinator should keep a mapping between member ID and group instance ID. Why is the group instance ID required when leaving? -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
lucasbru commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1500408843 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: I suppose this was copied from the KIP, so it's a question of the protocol specification. I don't think in the consumer implementation it can change. It seems that either that a little imprecision in the protocol spec, or the protocol allows changing/adding the instance ID in a running client. @dajac probably knows which of the two it is. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
mjsax commented on code in PR #15419: URL: https://github.com/apache/kafka/pull/15419#discussion_r1500165337 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java: ## @@ -546,8 +546,9 @@ public ConsumerGroupHeartbeatRequestData buildRequestData() { data.setMemberEpoch(membershipManager.memberEpoch()); // InstanceId - only sent if has changed since the last heartbeat Review Comment: For my own education? How can `group.instance.id` change? Isn't it provide by the user via a config, and configs are immutable? -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
lucasbru commented on PR #15419: URL: https://github.com/apache/kafka/pull/15419#issuecomment-1959188427 @lianetm @cadonna Could you take 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16152: Fix PlaintextConsumerTest.testStaticConsumerDetectsNewPartitionCreatedAfterRestart [kafka]
lucasbru opened a new pull request, #15419: URL: https://github.com/apache/kafka/pull/15419 The group coordinator expects the instance ID to always be sent when leaving the group in a static membership configuration, see https://github.com/apache/kafka/blob/ea9450767932eb3d63aeefd5af07dbc54cb92c31/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java#L814 The failure was silent, because the group coordinator does not log failed requests and the consumer doesn't wait for the heartbeat response during close. ### 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org