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
