Add jsonl streaming support #14898
Add jsonl streaming support #14898
Conversation
There was a problem hiding this comment.
How about if we make the format like this:
- line oriented
jsonl - each line is an object that has either
{"Item": {}}for data,{"Error": {}}for error or{"Stats": {}}for query statistics.
That match the format we have as well as make it easy to distinguish between different types of values we want to return.
|
|
||
| public void WriteQueryStatistics(long resultEtag, bool isStale, string indexName, long totalResults, DateTime timestamp) | ||
| { | ||
| throw new NotSupportedException(); |
There was a problem hiding this comment.
For streaming, we sent this at the start of the operation. That can be done via the first item (that the client will know to ignore? Not sure that I like that, to be honest. Maybe we can send that as headers?
There was a problem hiding this comment.
I was thinking about sending stats at the start of the operation, also was not sure what to make of it. I think the headers approach might be better now that you mention it. I guess the choice is down to what is easier for the clients to implement?
To add a second use-case as a potential tie-breaker, i was hoping to be able to pipe results directly into an another stream - say we have filtering/projections on raven and we want to respond directly back to a client over a RESTful API.
Another thought that comes to mind is that if we have to introduce a wrapper around individual items, then introducing headers as a second way to provide data might not be a good choice needed, as we need to process items anyway.
|
|
||
| public ValueTask WriteErrorAsync(string error) | ||
| { | ||
| throw new NotSupportedException(); |
There was a problem hiding this comment.
Note that errors should be in done in a way that would be breaking for the client.
What we want to ensure is that there is no way that they will accept that as a valid value.
There was a problem hiding this comment.
Can there be a mix of errors and results, or if there is an error then the only thing in the response is the error?
There was a problem hiding this comment.
If there is an error, then we report that and abort the rest of the operation
Do we need a wrapper for included results as well? |
|
For the included results, how would you tell if the value has an |
Not following the question, although I've remembered that include is not supported for streaming so i guess nothing needs to be done there? Otherwise, if we are in agreement, i would do what you suggested here
as i believe it would give us everything we need, which would be:
|
src/Raven.Server/Documents/Queries/StreamJsonlDocumentQueryResultWriter.cs
Show resolved
Hide resolved
nemanja-tosic
force-pushed
the
jsonl
branch
from
c904aa8
to
7b05637
Compare
|
Hi! Thank you for contributing into RavenDB project. Unfortunetely we detected that not all committers signed our CLA. Following contributors are not in our database:
Please visit http://ravendb.net/contributors/cla/sign to sign CLA. |
nemanja-tosic
force-pushed
the
jsonl
branch
from
7b05637
to
eaa2964
Compare
|
Signed the CLA, used a different email the first time around. |
Issue link
https://issues.hibernatingrhinos.com/issue/RDBC-622
Additional description
Support for JSON streaming, more specifically line-delimited JSON, for query streaming. The intent is to reduce the amount of work done by the clients when parsing JSON stream. So far while testing locally, it is at least 2x faster to process a JSONL response vs a JSON response in a stream.
Type of change
How risky is the change?
Backward compatibility
Is it platform specific issue?
Documentation update
Testing
AsyncBlittableJsonTextWriterneeds.Is there any existing behavior change of other features due to this change?
UI work