Skip to content
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

Response-only Streaming #555

Open
zizhong opened this issue Mar 12, 2021 · 0 comments
Open

Response-only Streaming #555

zizhong opened this issue Mar 12, 2021 · 0 comments

Comments

@zizhong
Copy link

zizhong commented Mar 12, 2021

Problem

In Rest.li, Stream doesn’t work well with the retry mechanism.
There are two usages so far.

  1. Backup Request. The Backup Request feature doesn’t work with stream requests. An exception raised because SetReader() is invoked twice for the same EntityStream. Here are the related tickets. The ticket reporter mentioned their requests are fully buffered and only need streaming for the response.
  2. RetryClient. The stream request will be fully recorded in order to replay it. There is no limit on the buffer size, so it can cause potential OOM. Retry is only enabled for specific requests and most of which is already fully buffered in memory.

Potential Fixes

  1. There is a retry logic in RetryClient for a stream request. The implementation is to use a FullEntityObserver to buffer the full request. Every time a request needs to be sent, the RetryClient will initiate a new instance of the stream request with buffered content. We can apply the same logic to the BackupRequestClient.
  2. It feels correct to only support retries for a fully buffered request. Before migrating R2 to full streaming mode, a fully buffered request is implemented as a rest request. We can introduce a new API where we allow the user to send a rest request and to receive a stream response. We will only support retrying for a rest request.

Decision

We agreed Option 2 is better, due to the following reasons.

  1. Semantically it’s correct to only support retries for a rest request and to disallow retries for a streaming request.
    Performance-wise, record-and-resend causes additional memory allocations, memory copies, and new instances of Objects including requests, entity streams, etc.
  2. The record-and-resent with stream requests could cause potential OOM if the request body is too large. A limit on buffer size is required. (which is missing for RetryClient.)
  3. Option 2 also simplifies the code by removing the conversion from RestRequest to StreamRequest in the case of rest_over_stream, and special checks around the header IS_FULL_REQUEST. So, it’s good to achieve better maintainability and slightly better performance.
@zizhong zizhong changed the title Response Only Streaming Response-only Streaming Mar 12, 2021
zizhong pushed a commit to zizhong/rest.li that referenced this issue Mar 12, 2021
zizhong pushed a commit to zizhong/rest.li that referenced this issue Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants