-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#555] Response-only Streaming #556
Conversation
a71203d
to
01ef9a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good. Can we also have integration tests written for these under R2-int-test module? Maybe for the below combinations with the RestRequestStreamResponse API
- Rest Echo and Stream Response
- Timeout
- Rest request Compression
- Stream Response Compression
import com.linkedin.r2.transport.http.client.common.ErrorChannelFutureListener; | ||
import com.linkedin.r2.transport.http.client.common.ssl.SslSessionValidator; | ||
import com.linkedin.r2.transport.http.client.stream.AbstractNettyResponseOnlyStreamClient; | ||
import com.linkedin.r2.transport.http.client.stream.AbstractNettyStreamClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import AbstractnettyStreamClient
* @author Steven Ihde | ||
* @author Ang Xu | ||
* @author Zhenkai Zhu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the author section since this is a new file
/** | ||
* Creates a new HttpNettyStreamClient | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a new ResponseOnlyStreamClient?
/** | ||
* @author Steven Ihde | ||
* @author Ang Xu | ||
* @author Zhenkai Zhu | ||
* @author Sean Sheng | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the @author section, since this is a new file
Put this work on hold as this requires either excessive refactor or additional flags. So that we decided to make the record-and-replay patch to resolve the issue quickly. |
Details can be found in #555.