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

Brings back support for StringData in ATS provider #8965

Merged
merged 3 commits into from May 23, 2024

Conversation

ledjon-behluli
Copy link
Contributor

@ledjon-behluli ledjon-behluli commented Apr 30, 2024

Currently the Azure Table Storage provider saves the grain state (encoded JSON) but in binary format- Regardless if the orleans serializer or one of the two json serializer are used. And while it can be decoded as UTF-8, its cumbersome to do that when someone wants to manually intervene and change the data in ATS. Let alone just a quick way to simply look at the data.

This PR brings back StringData but with the difference that its does NOT use Newtonsoft.Json to serialize the data as it was previously, but instead uses the GrainStorageSerializer and converts the BinaryData to a string format. This allows the data to follow what ever grain storage serializer is configured by the user i.e.: Newtonsoft.JsonorSystem.Text.Json`.

AFAIK this is not a breaking change, as we'll continue to read both formats for backwards compatibility.

Microsoft Reviewers: Open in CodeFlow

@ledjon-behluli
Copy link
Contributor Author

Pic of it in action

image

@@ -202,23 +203,29 @@ private static async Task DoOptimisticUpdate(Func<Task> updateOperation, string
/// </remarks>
internal void ConvertToStorageFormat<T>(T grainState, TableEntity entity)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to add a test to check the behavior of the new option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@ledjon-behluli
Copy link
Contributor Author

I'll address both points

@ledjon-behluli
Copy link
Contributor Author

@benjaminpetit i've addressed both points, sorry for the delay! Have a look at the test specifically the comment I left, let me know if my reasoning makes sense to you.

@ledjon-behluli
Copy link
Contributor Author

@ReubenBond it would be awesome if this could be merged :)

@ReubenBond ReubenBond merged commit 1d3f7d0 into dotnet:main May 23, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants