Skip to content

[WIP] Fix storage for generic grains in a way that is backward-compatible for normal grains - #1920

Closed
Maarten88 wants to merge 1 commit into
dotnet:masterfrom
Maarten88:genericgrainfix
Closed

Maarten88 wants to merge 1 commit into
dotnet:masterfrom
Maarten88:genericgrainfix

Conversation

@Maarten88

Copy link
Copy Markdown
Contributor

This change is in response to a question in #1915 from @jdom

For generic grains (which have a long partition key) this sets the rowkey to String.Empty so that ClearStateAsync works. Only ClearState was affected, it seems ReadState and WriteState worked with the long key.

I'm not sure about this one myself: it fixes the problem but I'm not sure I understand the Azure table storage behavior w.r.t how it deals with long keys containing special characters. A better fix would be to change the internal handling of generic arguments by removing the qualified assembly name, making it much shorter. Yet another option would be to change grainReference.ToKeyString() to return a shorter key for generic grains.

This change changes the storage key values for generic grains with previous versions. Normal grains should be unaffected.

@dnfclas

dnfclas commented Jul 8, 2016

Copy link
Copy Markdown

Hi @Maarten88, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@gabikliot

Copy link
Copy Markdown
Contributor

I read your comment on #1915 (comment), but I am still not convinced the right solution is just to return an empty string for Row Key for generic grains.
I think we should sanitize and shorten for generic case, just like you did for non generic case. There is no reason it would not work.
Note that Azure storage emulator has a different limit than actual service.
If for some weird reason sanitized and shortened Row key does not work, lets debug it. Lets print the length, find at which length it does work, ....
If we leave it this way we will of course get questions later about why this is different and will have problem to change the generic case due to backward compat with the empty row key case.

@gabikliot gabikliot added the bug label Jul 10, 2016
@Maarten88

Copy link
Copy Markdown
Contributor Author

@gabikliot I agree this needs more research. I only tested on the emulator and also feel this solution is poor. I do feel an empty rowkey is as good as anything, but having used the grain type until now makes it logical to continue doing that.

My vacation has started, I didn't bring my laptop so I can't work on this for the coming three weeks, sorry. I pushed this mostly to share my progress.

Feel free to ditch this pull request and find a better fix for this weird bug.

@sergeybykov sergeybykov changed the title Fix storage for generic grains in a way that is backward-compatible for normal grains [WIP] Fix storage for generic grains in a way that is backward-compatible for normal grains Jul 10, 2016
@sergeybykov

Copy link
Copy Markdown
Contributor

@Maarten88 Were you planning to return to this? Or should I close it?

@sergeybykov

Copy link
Copy Markdown
Contributor

@Maarten88 Closing for now. Please reopen if you get back to it.

@github-actions github-actions Bot locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants