Implement sidecar streaming read to accommodate the tracking report digest computing #64
Conversation
|
Hi, @ligangty could you help to review the code? This is a bug related to sidecar 2.0.0 which blocked the streaming reading. Thanks! |
ligangty
left a comment
ligangty
left a comment
There was a problem hiding this comment.
Generally looks good.
BTW, do we have a unit test for this ProxyStreamingOutput?
| logger.trace( "Copying from: {} to: {}", bodyStream, out ); | ||
| IOUtils.copy( bodyStream, out ); | ||
|
|
||
| OutputStream out = cout; |
There was a problem hiding this comment.
Is this out needed? Why not use cout directly here?
There was a problem hiding this comment.
@ligangty This is the same with the previously removed code, I think what John considered here might be out is used for writing, cout is used for bytes count.
| } | ||
| catch ( IOException e ) | ||
| { | ||
| if ( otel.enabled() ) |
There was a problem hiding this comment.
Is this possible to happen in the new okhttp3 way?
There was a problem hiding this comment.
@ligangty it's verified, IOException is never thrown in responsebody.close().
@ligangty no, sidecar doesn't have the ReportService and WebClientAdapter related unit tests, this streaming should call both of them for the testing, it needs to be perfect then. |
|
Ok, so it LGTM. |
2 participants
Update the implementation of ProxyStreamingOutput to handle ProxyService.get. We need bytes to read from the buffer to take on the responsibility for computing digest for the sidecar tracking report.
The buffer reading is different from #31 since we already change HTTP client into okhttp3. The okio BufferedSource could keep a buffer internally so that callers can do small reads without a performance penalty.
We could request the case from PNC to test the large streaming performance, from what I tested locally, it's stable so far.
Related bug: https://issues.redhat.com/browse/MMENG-3532