AndrewJSchofield commented on code in PR #19192:
URL: https://github.com/apache/kafka/pull/19192#discussion_r1993105204
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ShareConsumeRequestManager.java:
##########
@@ -1087,6 +1099,8 @@ public class AcknowledgeRequestState extends
TimedRequestState {
*/
private boolean isProcessed;
+ private final long deadlineMs;
Review Comment:
Javadoc please in line with all of the other members of this class.
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ShareConsumeRequestManagerTest.java:
##########
@@ -326,12 +328,10 @@ public void testCommitSync() {
networkClientDelegate.poll(time.timer(0));
assertTrue(shareConsumeRequestManager.hasCompletedFetches());
- Acknowledgements acknowledgements = Acknowledgements.empty();
- acknowledgements.add(1L, AcknowledgeType.ACCEPT);
- acknowledgements.add(2L, AcknowledgeType.ACCEPT);
- acknowledgements.add(3L, AcknowledgeType.REJECT);
+ Acknowledgements acknowledgements = getAcknowledgements(1, 3);
Review Comment:
This doesn't do the same thing. It uses to be `ACCEPT, ACCEPT, REJECT` and
now it's `ACCEPT, ACCEPT, ACCEPT`. You could argue that the acknowledge types
are not exploited in the test, which is I suppose correct. However, I do prefer
having a mixture like this because it has the opportunity for exercising extra
code paths and might open up the discovery of a bug down the line.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ShareConsumeRequestManager.java:
##########
@@ -1188,6 +1203,14 @@ boolean isEmpty() {
inFlightAcknowledgements.isEmpty();
}
+ /**
+ * Resets the timer with the new deadline and resets the RequestState.
Review Comment:
This comment says "new deadline" but I don't see the setting of a NEW
deadline in here. Please clarify.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
