Grain key type isn't saved with the key

[shayhatsor]

I'm currently trying to implement an IStorageProvider.
The problem I'm facing is the lack of a method to get the original key provided by the user.
AFAIK, Orleans flattens the key to UniqueKey which I don't see any way to convert back.
I've implemented a method that attempts to do that, but it assumes too much.
the problem is that a GUID that its first half is zeros is saved the same as a long.
the UniqueKey.IsLongKey always returns true if the first half is zeroes.
this is a bug, but fixing it will mean you can't use a long key equals to zero or a guid with all zeroes.
I thought about adding another enum value to UniqueKey.Category but that will break any current reminders kept without it.

[gabikliot]

The problem I'm facing is the lack of a method to get the original key provided by the user.

But there are extension methods on GrainReference (on IAddressable, which GrainReference implements) to get the underlying key, without type code. https://github.com/dotnet/orleans/blob/master/src/Orleans/Core/GrainExtensions.cs#L152

Cannot you use them?

[shayhatsor]

@gabikliot, the IStorageProvider I'm implementing must support any kind of grain key. but GrainReference doesn't know which key it was originally built with. The available extension methods assume the user knows which type of key they used to create the grain, this isn't the case inside an IStorageProvider.

[gabikliot]

Then maybe we should provide a way to query this info?
Returning key as a string is suboptimal compared to returning a strongly typed key, don't you think?

[gabikliot]

I mean, there are already like 5 ways to turn GR into a string. I think we should try to reduce those by providing a better way to extra the encapsulated info, rather then adding yet another ToXXXString.

[shayhatsor]

@gabikliot , I totally agree and I thought about that too.
that's why i said that we should add an enum value to UniqueKey.Category to first make the key itself save this information, change this:

   public enum Category : byte
        {
            None = 0,
            SystemTarget = 1,
            SystemGrain = 2,
            Grain = 3,
            Client = 4,
            KeyExtGrain = 6,
        }

into this:

   public enum Category : byte
        {
            None = 0,
            SystemTarget = 1,
            SystemGrain = 2,
            GuidKeyGrain = 3,
            Client = 4,
            KeyExtGrain = 6,
            LongKeyGrain = 7
        }

, and then this:

public interface IGrainIdentity
    {
        Guid PrimaryKey { get; }
        long PrimaryKeyLong { get; }
        string PrimaryKeyString { get; }
        string IdentityString { get; }
        long GetPrimaryKeyLong(out string keyExt);
        Guid GetPrimaryKey(out string keyExt);
    }

into this:

public interface IGrainIdentity
    {
        Guid GuidKey { get; }
        Nullable<long> LongKey { get; }
        string StringKey { get; }
        string Identity { get; }
    }

and then pass Grain.Identity to every IStorageProvider method called in GrainStateStorageBridge.

[sergeybykov]

This is an interesting problem that we've been pushing out for a long time. From the beginning a long key was considered a shortcut for a Guid with half of it set to zero. I think I like the idea of explicitly preserving the type of key that was used. I am not convinced though that the proposed expanded Category enum is a good solution. We have system targets and system grains that can also be used today with either a Guid or a long key. Maybe a separate flag would be a better solution that would avoid explosion of permutations in the Category enum? We could also use that flag to disambiguate compound vs. string-only key.

[shayhatsor]

@sergeybykov I agree, that was more of a sketch for the least changes needed. My only concern is current data saved with the "old" enum e.g. Reminders. But, correct me if I'm wrong, upon deserialization of GrainReference we get its TypeCodeData, from that we can find out the missing key type information.