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() {
state.reset();
assertTrue(state.canSendRequest(200));
}
+
+ @Test
+ public void testTrackInflightOnSuccessfulAttempt() {
+ testTrackInflight(RequestState::onSuccessfulAttempt);
+ }
+
+ @Test
+ public void testTrackInflightOnFailedAttempt() {
+ testTrackInflight(RequestState::onFailedAttempt);
+ }
+
+ /**
+ * In some cases, the network layer is <em>very</em> fast and can send out
a second request within the same
+ * millisecond timestamp as receiving the first response.
+ *
+ * <p/>
+ *
+ * The previous logic for tracking inflight status used timestamps: if the
timestamp from the last received
+ * response was <em>less</em> than the timestamp from the last sent
request, we'd interpret that as having an
+ * inflight request. However, this approach would incorrectly return
<code>false</code> from
+ * {@link RequestState#requestInFlight()} if the two timestamps were
<em>equal</em>.
+ */
Review Comment:
nit: Do we think it's helpful to explain so much about a "previous logic"
that was wrong here? I find it can be confusing for others. Also here we're
actually just testing the approach of checking inflights based on a flag (the
why we chose that approach makes more sense to me in the `requestInflight()`
func itself)
--
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
