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. 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. I think my
previous comments were confusing on what the behavior should be, 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. I think reporting the
request/responses sent per invocation by apiKey might be something we want in
the future, but we can add it later. It seems very nontrivial to do now,
because many `TestContext` helpers send RPCs.
In short, I think this should be two functions. The first 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 so the
state in.
--
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]
