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 should happen before we collect the request
deltas for the current invocation. Otherwise, the `TestContext` has outstanding
requests at the end of a `@Benchmark` invocation. We should call that method in
`drainFrom`. Also, we should be able to assert the `sendQueue` is empty at the
end of that method. Otherwise, that is a performance regression, because we're
sending more requests than we think. What do you think?
Additionally, this `lastRequestsSentByApiKey` logic from L208-211 seems
incorrect. 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.
We should have two functions probably, one that drains the request queue,
and asserts it is empty. That function can take in an `Optional<ApiKey>`
parameter if the benchmark writer expects outstanding requests. The second
would be this method, which returns the # of rpc requests sent delta.
--
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]
