Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2086027819 Thanks everyone for the reviews and @lucasbru for the merge! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

philipnee commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2085841809 Hey sorry for the delay, the changes look good to me. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru merged PR #15723: URL: https://github.com/apache/kafka/pull/15723 -- 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:

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2084640215 @philipnee Since the unresolved conversation is only around an internal method name, I'm merging this. Feel free to ask for a follow-up PR -- This is an automated message from the

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1583450102 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -98,12 +93,11 @@ public boolean canSendRequest(final long currentTimeMs)

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1579862658 ## clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestStateTest.java: ## @@ -48,4 +50,40 @@ public void testRequestStateSimple() {

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1579725251 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -98,12 +93,11 @@ public boolean canSendRequest(final long currentTimeMs)

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2076731964 Still looking good to me. I was waiting for the comments from philipp to be commented on / addressed. Do you want to skip ahead an merge? -- This is an automated message from the

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2071054599 @lucasbru, @lianetm, @philipnee—I have re-introduced the `lastSentMs` instance variable, not as a driver for logic, but for informational purposes in the logs. This is ready for

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

philipnee commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1571359996 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -98,12 +93,11 @@ public boolean canSendRequest(final long

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1571335784 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -98,12 +93,11 @@ public boolean canSendRequest(final long currentTimeMs)

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

philipnee commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1570988398 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -98,12 +93,11 @@ public boolean canSendRequest(final long

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

philipnee commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1570986831 ## clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestStateTest.java: ## @@ -48,4 +50,40 @@ public void testRequestStateSimple() {

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2063300823 @lianetm did you want to make another pass or good to go? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

philipnee commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1569554430 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/CoordinatorRequestManager.java: ## @@ -98,15 +98,15 @@ public NetworkClientDelegate.PollResult

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2059706910 @lianetm & @lucasbru—I've addressed your comments, and this is ready for another review. Thanks! -- This is an automated message from the Apache Git Service. To respond to the

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1567791850 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -66,6 +67,7 @@ public RequestState(final LogContext logContext, *

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1567722373 ## clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestStateTest.java: ## @@ -48,4 +50,51 @@ public void testRequestStateSimple() {

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1567484668 ## clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestStateTest.java: ## @@ -48,4 +50,51 @@ public void testRequestStateSimple() {

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lianetm commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2059130202 Thanks for the changes @kirktrue , LGTM. Just thinking out loud to share a concern and reasoning about it. The flag based approach always makes me think if we would break managers

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lianetm commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1567368858 ## clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestStateTest.java: ## @@ -48,4 +50,51 @@ public void testRequestStateSimple() {

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

lucasbru commented on code in PR #15723: URL: https://github.com/apache/kafka/pull/15723#discussion_r1567025166 ## clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestState.java: ## @@ -66,6 +67,7 @@ public RequestState(final LogContext logContext, *

Re: [PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue commented on PR #15723: URL: https://github.com/apache/kafka/pull/15723#issuecomment-2057988468 @lucasbru—please kindly take a look at this issue that's occasionally causing duplicate heartbeat requests. cc @lianetm @philipnee -- This is an automated message from the

[PR] KAFKA-16555: Consumer's RequestState has incorrect logic to determine if inflight [kafka]

kirktrue opened a new pull request, #15723: URL: https://github.com/apache/kafka/pull/15723 In some cases, the network layer is _very_ fast and can send out multiple requests within the same millisecond timestamp. The previous logic for tracking inflight status used timestamps: if