Reusable storage bridge by jason-bragg · Pull Request #3360 · dotnet/orleans

jason-bragg · 2017-08-30T01:38:48Z

Refactored storage bridge and stateful grain handling to allow more flexable state management needed by transaction logic.

Removed grain state managment from catalog and grain creator.
Grain<> and LogConsistancy grain now setup their own state via the lifecycle.
GrainStateStorageBridge has been replaced with StateStorageBridge which can be used for transaction and storage facet implementations which choose to store state using storage providers.

Note: With this change, only type managment, and statistics issues prevent event sourcing and stateful grain (grain<>) from being moved out of orleans core/runtime into orleans extension libraries.

See Orleans Transactions #3369

jason-bragg · 2017-08-30T01:51:31Z

Moving state management logic out of catalog and grain-creator into grain base classes that participate in the lifecycle is the bulk of this change. While we should be moving away from base classes, this preserves the existing behaviors and moves this logic out of the catalog/grain-creator and provides a more extensible pattern for future features.

ReubenBond · 2017-08-30T01:53:09Z

suggest we make this an explicit implementation so as not to pollute intellisense for Grain

ReubenBond · 2017-08-30T23:58:00Z

Doesn't need to change in this PR, but there's an extension method called TryAddFromExisting which essentially does this.

ReubenBond · 2017-08-31T00:04:54Z

One change to consider is that when a lifecycle participant fails, propagate the original exception to the caller instead of an OperationCanceledException.

Highlighted in the failure of Persistence_Grain_Activate_Error:
MESSAGE: Assert.Throws() Failure\r\nExpected: typeof(UnitTests.StorageTests.StorageProviderInjectedError)\r\nActual: typeof(System.OperationCanceledException): Lifecycle start canceled due to errors at stage SetupState

That change had no effect until this PR, but I believe it was already in place.

galvesribeiro

LGTM. Great work!

jdom · 2017-08-31T17:47:48Z

you probably meant to use grainTypeName as a parameter here

jdom · 2017-08-31T17:52:23Z

Implementing this explicitly would mean that app devs (or infrastructure extensions) will not be able to create a base class that is a participant and hook it up as expected, correct? Maybe it should just be protected virtual instead?

jdom · 2017-08-31T17:54:59Z

maybe the ILifecycleObserver itself can be instantiated on the Participate phase, so that the grain itself doesn't have to implement ILifecycleObserver that could conflict with how the users implement their grains?
For example the StateStorageBridge itself could implement that interface, or an inner class of it, or even have it implicitly implemented with a lambda that can use closures.

jdom · 2017-08-31T18:13:11Z

I think this is not backwards compatible. In the past we were using $"{grainType}[{genericArguments}] as the state name, where grain type is not even type.FullName or something like that.

hmm.. looks like there was a bug..? We load the initial state using what you describe, but the storage bridge was setup with a different name, meaning the initial loaded state is different from the state the bridge uses. Not sure how that got past tests, but that's a serious bug, unless I'm misunderstanding what I'm reading.

Change was introduced in July of last year, so it may be lower impact than I'd expected. f634bde
Anyone familiar with what this was for? @ReubenBond? @jdom?

Ah. I see. This addressed a problem which only existed because the bridge was not being used to read the initial state. Current code should be fine.

jdom · 2017-08-31T18:20:16Z

question: shouldn't this happen before calling activation.Lifecycle.OnStart();

The expected behavior is that state be loaded then pre-activate, activate, post-activate. Since state load happens in lifecycle, this will need to be after onStart, for now. When onActivateAsync is called by the lifecycle, this can be as well.

jdom · 2017-08-31T18:23:30Z

Yes, I was even considering that the Grain.OnActivateAsync could actually be implicitly hooked up by the Grain base class being a lifecycle participant, not as a special method that we handle separately in the catalog.
This way, we get closer to the goal of supporting POCO objects as grains in the future, if all that is in Grain can be implemented by scoped activation services and lifetime observers.

That is the plan. All of grain lifecycle should be handled within the lifecycle framework. We've just not refactored it all yet. A good next step would be to add pre-activate, activate, and post-activate stages to the lifecycle, and move the onactivate/deavtivate within the lifecycle (as you've describe) along with the pre/post activate logic. Though, I'm not convinced we need the pre/post activate, so I'd like to understand why that was needed.

jdom · 2017-09-01T14:58:06Z

hmmm, not sure I like the idea of adding this public extension method to all grains. Do we really expect users to find this easily when developing grains?

It's related to storage provider usage. We can put it into a storage specific extension if we want. As for expectation, I don't expect many users will need nor use this capability. This function affords grain developers a common way of obtaining a storage provider for a grain (as needed by Grain<> and LogConsistentGrain, or other such grains that wish to use storage providers), as well as improves the testability of storage provider to grain mapping.

Suggestions?

At the very least I would constrain it to Grain<T>, not every Grain, but I think I prefer in something different than GrainExtensions that doesn't pollute the intellisense (or even not provide an extension method for this).
As you mentioned it's probably only going to be used by some infrastructure code (even just from us). And as we endorse facets, this is even less relevant.

probably missed addressing this one

Didn't fix because there was nothing broken. Moved to namespace Orleans.Storage anyway, so it won't show up unless someone is already using storage features.

jdom · 2017-09-01T15:00:11Z

nit: add the private modifier

jdom · 2017-09-01T15:03:56Z

This would completely wipe out the original stack trace of the exception, which is very important for troubleshooting what went wrong. You might want to do the following instead:

ExceptionDispatchInfo.Capture(canceledException.InnerException).Throw();

jdom · 2017-09-01T15:06:40Z

Please undo this change after rebasing on top of master, since we removed all NETSTANDARD conditional compilation statements in #3368 that was just merged

jason-bragg · 2017-09-01T18:24:46Z

@dotnet-bot test netfx-functional please

Refactored storage bridge and stateful grain handling to allow more flexable state management needed by transaction logic. - Removed grain state managment from catalog and grain creator. - Grain<T> and LogConsistancy grain now setup their own state via the lifecycle. - GrainStateStorageBridge has been replaced with StateStorageBridge which can be used for transaction and storage facet implementations which choose to store state using storage providers. Note: With this change, only type managment, and statistics issues prevent event sourcing and stateful grain (grain<T>) from being moved out of orleans core/runtime into orleans extension libraries.

jason-bragg · 2017-09-01T20:59:20Z

Squashed rebased.
Addressed all comments.

…show up in intelisense unless one is using storage.

jason-bragg · 2017-09-05T23:11:33Z

ping

jdom · 2017-09-05T23:34:20Z

Oh, sorry, didn't merge as I thought you found a big issue with the name we use as state key. Was this resolved?

ReubenBond · 2017-09-05T23:35:44Z

Oh, I thought that was the existing behavior which was just being carried forwards

From the comment:

Ah. I see. This addressed a problem which only existed because the bridge was not being used to read the initial state. Current code should be fine.

jdom · 2017-09-05T23:52:54Z

If that really existed, it would be a huge issue that needs to be investigated and potentially addressed asap (not just for 2.0). That's why I thought he was still investigating it, because it's crazy that no one encountered this before

jdom · 2017-09-06T00:32:30Z

@jason-bragg can you confirm?

jason-bragg · 2017-09-06T17:53:59Z

TL,DR: The original code was correct. The new code is correct.

Full description of original problem: "Incomplete grainType when activating generic grains #1579"
Fix: "Fix bug with generic state parameter caused by inconsistent use of grainClassName / genericArgument / genericInterface #1897"

The difference in state name generation in the catalog vs. the bridge is due to the fact that the catalog used the type from the grain type data which did not have the generic value which is only known at runtime. The bridge did not need to deal with this problem because it used the fully qualified runtime grain type. By removing the (duplicate) load logic from the catalog, we no longer use the unqualified type from the grain type data, so the issue goes away.

dnfclas added the cla-already-signed label Aug 30, 2017

jason-bragg commented Aug 30, 2017

View reviewed changes

ReubenBond reviewed Aug 30, 2017

View reviewed changes

jason-bragg mentioned this pull request Aug 30, 2017

Orleans Transactions #3369

Closed

ReubenBond reviewed Aug 30, 2017

View reviewed changes

ReubenBond approved these changes Aug 30, 2017

View reviewed changes

galvesribeiro approved these changes Aug 31, 2017

View reviewed changes

jdom reviewed Aug 31, 2017

View reviewed changes

jason-bragg force-pushed the StorageRefactor branch from c6961a8 to d1808d8 Compare August 31, 2017 21:07

jdom reviewed Sep 1, 2017

View reviewed changes

jason-bragg force-pushed the StorageRefactor branch from c53c3ee to 256b67a Compare September 1, 2017 19:45

jdom approved these changes Sep 1, 2017

View reviewed changes

Moved GetStorageProvider to namespace Orleans.Storage so it does not …

849a926

…show up in intelisense unless one is using storage.

ReubenBond merged commit 43c81d0 into dotnet:master Sep 5, 2017

github-actions Bot locked and limited conversation to collaborators Dec 10, 2023

Uh oh!

Conversation

jason-bragg commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReubenBond commented Aug 31, 2017

Uh oh!

galvesribeiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-bragg Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-bragg commented Sep 1, 2017

Uh oh!

jason-bragg commented Sep 1, 2017

Uh oh!

jason-bragg commented Sep 5, 2017

Uh oh!

jdom commented Sep 5, 2017

Uh oh!

ReubenBond commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdom commented Sep 5, 2017

Uh oh!

jdom commented Sep 6, 2017

Uh oh!

jason-bragg commented Sep 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jason-bragg commented Aug 30, 2017 •

edited

Loading

jason-bragg Aug 31, 2017 •

edited

Loading

ReubenBond commented Sep 5, 2017 •

edited

Loading