Add serialization service support #840
Conversation
- Added a method that handles serialization requests - Support splitting save over multiple requests to reduce overall message size - Added unit tests Tests for Excel are not currently added as they're more cumbersome to write than others
String changes used a new version of the string tool for generation. Will publish PR separately for the changes to build & localization so this can run on Mac without .Net Core 1.0
- This had been added in error
src/Microsoft.SqlTools.ServiceLayer/QueryExecution/SerializationService.cs
Outdated
Show resolved
Hide resolved
| if (serializeParams.IsComplete) | ||
| { | ||
| this.CloseStreams(); | ||
| } |
There was a problem hiding this comment.
This seems odd to do the cleanup here - wouldn't most things only call this request once? I would expect that after serialization is complete the streams would be automatically closed
There was a problem hiding this comment.
Ok so looking at it more it seems like this will get multiple requests in batches for a single file (which makes sense). I guess this is fine but I'm not completely sold on the IsComplete name - IsLast seemed to make more sense to me. But IsLast isn't great either - maybe IsLastBatch? I'd also put a comment about keeping the streams open until we get told that all the data is transferred since it's otherwise unclear that this is a batched operation.
In reply to: 309756726 [](ancestors = 309756726)
There was a problem hiding this comment.
Ack, will consider renaming.
| DataSerializer serializer = null; | ||
| bool hasSerializer = inProgressSerializations.TryGetValue(serializeParams.FilePath, out serializer); | ||
| if (!hasSerializer) | ||
| { |
There was a problem hiding this comment.
Are requests actually multithreaded? If so this has a small chance for a race condition - if there isn't a serializer when we set hasSerializer but then another thread comes in and makes one before AddOrUpdate is called then we'll end up having two serializers for the same file. And generally this kind of check is an anti-pattern anyways.
I'd just create the new serializer and use TryAdd (ignoring if it already exists), simpler and avoids the possible race condition
There was a problem hiding this comment.
So my current thinking here is that the client will wait on the result from each call before sending more across. I believe given this, it should be fine.
I think to fix, we'd want to separate into 2 messages (start and update) and fail on start if another serializer existed. That would handle cases, for example, where folks clicked save twice. Let me take a look at this and think on it - this may be more robust against multiple requests.
Add serialization service support
String changes used a new version of the string tool for generation.
Will publish PR separately for the changes to build & localization
so this can run on Mac without .Net Core 1.0
Not covered yet: