kevin-wu24 commented on code in PR #22669:
URL: https://github.com/apache/kafka/pull/22669#discussion_r3484997654
##########
raft/src/testFixtures/java/org/apache/kafka/raft/RaftClientBenchmarkContext.java:
##########
@@ -162,10 +194,21 @@ public int drainLogTruncations() {
return delta;
}
- public int drainRpcRequestsSent() {
- int current = channel.requestsSent();
- int delta = current - lastRequestsSent;
- lastRequestsSent = current;
+ /**
+ * Number of requests sent since the last drain. If {@code apiKey} is
present, only requests of
+ * that API key are counted; otherwise all requests are counted.
+ */
+ public int drainRpcRequestsSent(Optional<ApiKeys> apiKey) {
+ if (apiKey.isEmpty()) {
+ int current = channel.requestsSent();
+ int delta = current - lastRequestsSent;
+ lastRequestsSent = current;
+ return delta;
+ }
+ short id = apiKey.get().id;
+ int current = channel.requestsSent(apiKey.get());
+ int delta = current - lastRequestsSentByApiKey.getOrDefault(id, 0);
+ lastRequestsSentByApiKey.put(id, current);
Review Comment:
Seems like we are missing a call to
`MockNetworkChannel#drainSentRequests(Optional<ApiKeys> apiKeyFilter)` before
entering this method, since that needs to happen before we collect the request
deltas for the current invocation. We should call that method in `drainFrom`.
Also, we should be able to assert the `sendQueue` is empty at the end of this
method. Otherwise, that is a performance regression, because we're sending more
requests than we think. What do you think?
This is so we can drain any remaining sent requests after the benchmarking
code calls `poll()` for the last time in the current invocation. Only after
this will our delta collection be accurate. This is similar to the current
implementation of `drainRpcResponsesSent`. What do you think?
Additionally, this `lastRequestsSentByApiKey` logic from L208-211 is going
to be incorrect. Basically, we're not going to count any other requests sent
over the benchmarking invocation if we pass an `apiKey` to this method. Maybe
my previous comments about this weren't very clear, and I think the naming
convention of these methods contributed to it. I think this was the comment:
https://github.com/apache/kafka/pull/22669#discussion_r3475499363? Since we are
only reporting the `requestsSent/invocation` at the end, this
`lastRequestsSentByApiKey` state is not necessary.
--
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]
