Check for errors during performance test by checking HTTP status. #19
Conversation
Previously ProducerPerformance read, but ignored the response entity and ConsumerPerformance didn't read the response entity when the HTTP method was DELETE, so both had conditions where they could miss errors. Although this doesn't check for specific, expected statuses, checking for any >= 400 should catch any important errors.
|
@granders Can you review this when you have a chance -- there's nothing really kafka-rest specific and you're now the other expert on the perf code. |
|
Seems like a good idea to check for http error codes. Both these request blocks have some overall similarity to/convergence with RestUtils.httpRequest in schema registry. It might make sense to refactor and share httpRequest in the common package. Doubly so since the behavior of HttpUrlRequest API is (to me at least) quite unintuitive. |
|
Agreed, I don't like the duplicated code. But I'm not yet sure what that utility would look like without duplicating a lot of the interface of HttpUrlConnection. At least a few things vary across the different implementations -- setting headers, how error codes are checked, and how responses are decoded. We might want to file an issue on confluentinc/common to provide a good wrapper or set of wrappers. |
|
+1 |
Check for errors during performance test by checking HTTP status.
Previously ProducerPerformance read, but ignored the response entity and
ConsumerPerformance didn't read the response entity when the HTTP method was
DELETE, so both had conditions where they could miss errors. Although this
doesn't check for specific, expected statuses, checking for any >= 400 should
catch any important errors.