MINOR: Factor out common response parsing logic#9617
Conversation
|
This is a follow-up from #9563. cc @chia7712 @abbccdda While I was working on this patch, I noticed this: https://issues.apache.org/jira/browse/KAFKA-10743. In this patch, I have just removed the metric. |
abbccdda
left a comment
abbccdda
left a comment
There was a problem hiding this comment.
Overall LGTM, just some minor comments
| * Raised if the correlationId in a response header does not match | ||
| * the expected value from the request header. | ||
| */ | ||
| public class CorrelationIdMismatchException extends IllegalStateException { |
There was a problem hiding this comment.
nit: we could put this exception into common/internals to indicate that we don't plan to let external user catch it.
There was a problem hiding this comment.
Yeah, I was debating the location. I decided to put it in common/requests since it is not a public package and the error is specific to the request protocol.
| } | ||
|
|
||
| public int requestCorrelationId() { | ||
| return requestCorrelationId; |
There was a problem hiding this comment.
Yeah, I added it for completeness. It is a little odd if the mismatch exception only tells you what the response correlationId was.
| /** | ||
| * Validate that the response corresponds to the request we expect or else explode | ||
| */ | ||
| private static void correlate(RequestHeader requestHeader, ResponseHeader responseHeader) { |
There was a problem hiding this comment.
nit: I don't think we need to remove this helper
There was a problem hiding this comment.
Why do you want to keep it if it is not used?
3 participants
This patch factors out some common parsing logic from
NetworkClient.parseResponseandAbstractResponse.parseResponse. As a result of this refactor, we are now verifying the correlationId in forwarded requests. This patch also adds a test case to verify handling in this case.Committer Checklist (excluded from commit message)