Eliminated GrainState

[dVakulen]

#1035.

Implemented POCO state, added etag to the SP domain, available for the further discussion.
The API of IStorageProvider changed to support writes with eTag.

[ReubenBond]

Great work! I was just starting to implement this myself! Please see the comments as well as: #1035 (comment). Thank you for this :)

[galvesribeiro]

Great work! Impressive how an issue that we was discussing in less than a week was already implemented! Keep pushing @dVakulen ! 😄

[sergeybykov]

This is big. Thank you for taking on it, @dVakulen! I'll take a closer look at it and try to test it tomorrow. I know @johnazariah will be very happy. :-)

[dVakulen]

Updated accordingly to the #1035 (comment)

[dVakulen]

If backward compatibility is needed, GrainState can be marked as deprecated.

[sergeybykov]

Yes, let's mark it deprecated and leave for a release or two.

[shayhatsor]

I totally missed this PR :)
Just wanted to make sure we're not missing anything, I've changed my comment #1035 (comment) because I believe my previous version possibly won't work bcz IStatefulGrain contains a method that doesn't return a Task and a property. what I'm not sure is whether codegen is done on explicit interface implementations.

[ReubenBond]

@shayhatsor I like the idea of Type instead of stringly grainTypeName. Why do we have an ETag in there, though? If instead the provider specifies the IStorage/IGrainState implementation, it can associate whatever context it wants with the grain.

Provider remains stateless, no assumptions on what information the provider needs to associate with the grain. We could have them specify a Type instead of returning an instance.

What are the drawbacks?

[shayhatsor]

@ReubenBond I think I get what you're saying. This is what seems logical to me at this point:

• A grain that has a state should be defined Grain<T> where there are no constraints on T.
• The grain should be storage provider agnostic. We should remove Etag from the grain state, It's an IStorageProvider implementation detail that shouldn't leak anywhere else.
• Like you have previously suggested, there should be an IStorageProvider instance for each grain instance. This decouples the state from its concurrency control by allowing the storage provider implementation to keep any data locally (like an ETag) knowing it serves a single grain.

In conclusion:

public interface IGrainState
{
    object State { get; set; }
}

public class GrainState<T> : IGrainState
{
    public T State { get; set; }

    object IGrainState.State
    {
        get { return State; }
        set { State = (T) value; }
    }
}

public class Grain<T> : Grain
{
    protected Grain()
    {
        GrainState = new GrainState<T>();
    }

    protected Grain(IGrainIdentity identity, IGrainRuntime runtime, GrainState<T> state, IStorage storage)
        : base(identity, runtime)
    {
        GrainState = state;
        Storage = storage;
    }

    protected T State
    {
        get { return (T) GrainState.State; }
        set { GrainState.State = value; }
    }

    protected virtual Task ClearStateAsync()
    {
        return Storage.ClearStateAsync();
    }

    protected virtual Task WriteStateAsync()
    {
        return Storage.WriteStateAsync();
    }

    protected virtual Task ReadStateAsync()
    {
        return Storage.ReadStateAsync();
    }
}

public interface IStorageProvider : IProvider
{
    Logger Log { get; } 
    Task ReadStateAsync(Type grainType, GrainReference grainReference, IGrainState grainState);
    Task WriteStateAsync(Type grainType, GrainReference grainReference, IGrainState grainState); 
    Task ClearStateAsync(Type grainType, GrainReference grainReference, IGrainState grainState);
}

[jason-bragg]

there should be an IStorageProvider instance for each grain instance

I believe we can improve the storage model by raising the abstraction a little higher. The grain actually talks to an IStorage class rather then the storage provider.

public interface IStorage
{
    Task ClearStateAsync();
    Task WriteStateAsync();
    Task ReadStateAsync();
}

This class acts as a bridge between the grain and the storage provider. The storage provider interfaces changes are a change to the contract with this class, not the grain. Instead of creating a provider per grain, I propose we request the IStorage object from the provider instead.

public interface IStorageProvider : IProvider
{
    IStorage GetStorage(GrainReference grainReference, IGrainState  grainState);
}

During grain construction we can obtain the storage object from the storage provider, then use it to store the state. Because we've raised the abstraction layer (to the IStorage interface), older storage providers can still be maintained with the current IStorage implementation (GrainStateStorageBridge), and future providers can implement any type of state to storage translation they like.

This also allows storage providers to take advantage of per grain storage constructs for things like caching layers and the like.

[jason-bragg]

Getting the IStorage implementation from the storage provider also affords us a natural extension point for more specialized storage patterns. Consider an event sourcing storage provider something like the following:

public interface IEventSourceStorage : IStorage
{
    Task SaveCheckpoint();
    Task LoadCheckpoint();
}

public interface IEventSourceStorageProvider : IStorageProvider 
{
    IEventSourceStorage GetEventSourceStorage(GrainReference grainReference, IGrainState  grainState);
}

This storage provider could be used as a regular storage provider, while allowing grains using event sourcing to use the more advanced features.

This is a general argument, I've know idea if those calls make sense for event sourcing :)

[ReubenBond]

@jason-bragg sounds good to me. I was trying to say pretty much that:

If instead the provider specifies the IStorage/IGrainState implementation, it can associate whatever context it wants with the grain.

I support your approach 👍

[shayhatsor]

@jason-bragg 👍

[shayhatsor]

In order to make the interface as clean as possible, I believe we shouldn't pass GrainReference at all. AFAIK it's only used to get the Orleans grain key, and it doesn't provide the user provided key (see #1068). If UniqueKey is fixed to save the user provided key type, the data can be exposed by a revised IGrainIdentity:

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

so the interface can be:

public interface IStorageProvider : IProvider
{                                             
    IStorage GetStorage(Type grainType, IGrainIdentity grainIdentity, IGrainState grainState);
}                    

what do you think @jason-bragg and @ReubenBond ?

[jason-bragg]

If I understand correctly, in Orleans, a grain reference is a grain's identifier. Creating a new id type would be redundant. Can you be more specific regarding your concerns about grain references?

@gabikliot and @sergeybykov are more versed on the intended usage of grain references.

[shayhatsor]

@jason-bragg, there's already an IGrainIdentity in Orleans. I was suggesting we'd change it from:

   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);
    }

to

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

so the IStorageProvider would be able to get the original key given to the grain.

[jdom]

+1 to @jason-bragg suggested abstraction. I'm not very fond of the very generic IStorage name, and maybe we can find something better, but I like the approach.
Perhaps IRepository? or IGrainRepository? or IGrainStorage? or IGrainStateStorage? or IStorageAdaptor (yuck)? Just throwing some names out there so that someone can piggyback and tell us what name would fit better here.

[jdom]

BTW, regarding naming, I didn't realize that IStorage already exists in Orleans in the way we want it, so it doesn't make sense to change the name, I thought it was a new interface we were going to add

[sergeybykov]

@shayhatsor

@jason-bragg, there's already an IGrainIdentity in Orleans.

IGrainIdentity is only identity from the target grain perspective as in "who am I". It's not a full identity because it doesn't include the grain class information (grain type code). GrainReference is the full global identity of a grain. Yeah, it sounds confusing, I realize.

[shayhatsor]

@sergeybykov, I know, that's why I asked to add the real grain type. I don't see how grain type code is useful outside of Orleans.

[sergeybykov]

I believe I addressed the serializer generation issue with #1211.

[gabikliot]

LGTM on my side.

[sergeybykov]

I kicked off tests of this combined with the latest version of #1211. If everything looks good and #1211 is merge, I'll merge this one.

[ReubenBond]

I believe we only partially addressed the serializer generation in #1211 - couldn't a user have a GrainImpl<T> : IMyGrain<T> and skip serializer gen by passing some unknown type at runtime?

[sergeybykov]

@ReubenBond The check I added is for all classes that extend Grain<T>. Did you rather mean that we'll miss generic grain classes GrainImpl<T> : Grain<T>?

[ReubenBond]

@sergeybykov if the client calls:
grainFactory.GetGrain<IMyGrain<SomeUnknownType>>(0) and IMyGrain<T> is implemented with the signature class MyGrain<T>: Grain<T>, IMyGrain<T>, then we may not have generated a serializer for SomeUnknownType, since it's specified by the client at runtime.

EDIT: that's not a blocking issue, it's not an issue introduced by this change, and users can always use [KnownType(typeof(T))] to ensure serializers are generated.

[gabikliot]

and users can always use [KnownType(typeof(T))] to ensure serializers are generated.

Or by marking SomeUnknownType as [Serializable].

[sergeybykov]

I think this is a bug - instead of nulling State we need to set it to a fresh instance of the state class.

Grain_AzureStore_Delete test is failing because of this. It tries to read a value of a state property right after calling ClearStateAsync, and the grain code throws a NullReferenceException.

[sergeybykov]

@ReubenBond In general, our generation of serializers is a best effort attempt. We can never guarantee that the app won't pass an unseralizable object, a subclass or something. So I agree, this is not really an issue with this PR, just a general limit to our serialization 'magic'.

[sergeybykov]

Persistence_Provider_Error_AfterRead test is also failing consistently. It's not immediately obvious to me why but it runs fine in master.

[dVakulen]

Fixed.

[sergeybykov]

This version pass all tests. So I'll merge it.

@dVakulen Thank you so much for the important contribution and the patience to get it in!

[galvesribeiro]

Weeee! Thanks @dVakulen! Great work! :) 👏

[gabikliot]

Thanks @dVakulen !

[veikkoeeva]

@dVakulen My thanks too. 👏

[dVakulen]

Thanks for your, @sergeybykov, @gabikliot, @ReubenBond, patience and reviews, and all the other participants for ideas.