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

fix: bind IAddressable instances to runtime when deserializing from JSON #10 #11

Merged
merged 11 commits into from
Aug 11, 2021

Conversation

heikki-heikkinen-ptcs
Copy link
Contributor

Uses Orleans JSON serialization settings instead of copy-pasting the same code to this library. Orleans.Core was already referenced.
Extracts serialization to/from RedisValue behind an interface where the implementation is selected when bootstrapping.
Attempts to fix migration from old implementation where state was stored as a plain key without etag to hash based structure.
Adds extension point to inject custom JSON converters to the JSON serialization settings.

@heikki-heikkinen-ptcs
Copy link
Contributor Author

The format selection in RedisGrainStorage.GetKey (var format = _options.UseJson ? "json" : "binary";) does not belong in that class any more, will fix on monday.

@heikki-heikkinen-ptcs
Copy link
Contributor Author

What is wrong with the CI? The test stage takes hours and then gets canceled?

@ReubenBond
Copy link
Contributor

I'm not sure @heikki-heikkinen-ptcs - does this work locally for you?

@ReubenBond
Copy link
Contributor

By the way, have you seen dotnet/orleans#6936?

@heikki-heikkinen-ptcs
Copy link
Contributor Author

Yes, test pass locally. They are a bit heavy since they require an actual redis instance to be running. I would strongly recommend checking out the branch while reviewing and running it locally. You tend not to see enough of the context when only looking at the diff.

And no, I have not seen that. I'll have to check it out.

@ReubenBond
Copy link
Contributor

Those CI tests run against a Redis container.

Tests were timing out in PR build. Removing new PubSub tests to make sure they were not the culprit
@heikki-heikkinen-ptcs
Copy link
Contributor Author

@ReubenBond I removed the test class I added and tests seem to pass now.

@heikki-heikkinen-ptcs
Copy link
Contributor Author

Resolved conflicts with target branch. Is this ever going to get reviewed?

@ReubenBond
Copy link
Contributor

Many apologies, @heikki-heikkinen-ptcs, I completely missed this.

@ReubenBond ReubenBond merged commit 827b85f into OrleansContrib:master Aug 11, 2021
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