Add storage exception injection logic into fault injection test runner #4874
Conversation
| { | ||
| void BeforeStore(); | ||
| void AfterStore(); | ||
| } |
There was a problem hiding this comment.
Seems odd that a fault injector would be added into the public abstractions package... is there a better way?
Eg, could we generalize this to ITransactionStorageObserver or a name with a similarly general meaning?
There was a problem hiding this comment.
Even better, could this be implemented using something like a decorator pattern instead? It seems that we could decorate ITransactionalStateStorage<TState> as-needed and avoid special-casing the code. As a bonus, doing so would remove the class of errors whereby we forget to call the Before/After methods around the Store call.
There was a problem hiding this comment.
the storage exception injection logic is too deep into the implementation, if using decorator pattern, I might need to decorate every class.. I will see what I can do there. may decorate ITransactionalStateStorage is a good alternative. But then we need to decorate different storage provider implementation if we have multiple
|
@sebastianburckhardt this is the failed storage exception injection tests I mentioned in the meeting. You can see more on what I described in the meeting here |
| long? commitUpTo, | ||
|
|
||
| // if non-null, abort all pending transactions with sequence numbers strictly larger than this one. | ||
| long? abortAfter |
There was a problem hiding this comment.
Usually doc comments like these would go above the method, like:
/// <summary>
/// Register a timer to send regular callbacks to this grain.
/// This timer will keep the current grain from being deactivated.
/// </summary>
/// <param name="asyncCallback"></param>
/// <param name="state"></param>
/// <param name="dueTime"></param>
/// <param name="period"></param>
/// <param name="name"></param>
/// <returns></returns>You could include this info there.
| public class FaultInjectionAzureTableTransactionStateStorageFactory : ITransactionalStateStorageFactory, | ||
| ILifecycleParticipant<ISiloLifecycle> | ||
| { | ||
| private readonly AzureTableTransactionalStateStorageFactory factory; |
There was a problem hiding this comment.
Could these be made more generic (I don't know), by resolving types using a different name?
eg, FaultInjectionTransactionStateStorageFactory takes some named options which tell it the name of the factory to use to resolve the transactional storage.
Or similarly, FaultInjectionTransactionStateStorage<TState> could take those options and do the resolution.
That way, we don't need new classes for each one.
As an alternative to options, we could use convention-based registration, like $"{name}_inner" for the type being resolved.
There was a problem hiding this comment.
We could probably make this more generic, but the fault tolerance of the system should, in theory, be storage independent, so we should really only need to run the storage fault injection tests against a single storage to identify the issues. If we do have a more generic need in the future, we can generalize this at the time.
There was a problem hiding this comment.
It probably can be made more generic, but currently there's only one type of fault injection tx state used. So more generic version of this doesn't have much usage now. We can make it more generic when we have more than 1 testTxState used in tests.
| [SkippableFact] | ||
| public async Task SingleGrainReadTransaction() | ||
| { | ||
| const int expected = 5; |
There was a problem hiding this comment.
This is a nit, but I believe .NET naming conventions calls for capitalizing the first letter in constant variable declarations.
|
@xiazen, @ReubenBond Could this point out some insight on how to add fault injection to Orleans in general? It looks to me it would be a useful feature together with #4805 and maybe the overall design would/could converge or take account these. |
|
@veikkoeeva generally, you can inject faults using grain call filters |
|
@dotnet-bot test functional please |
|
Needs rebase |
xiazen
force-pushed
the
storage-fault-injection
branch
from
494f28c to
56b63b8
Compare
|
rebased and squashed for easy rabse. waiting for tests. |
Adding fault injection test case
injecting exception before store simulates situation that state update failed to persist
injecting exception after store simulates situation that state update succeed to persist, but runtime think it failed
currently all tests failed... I don't think I have a test logic issue. But please give me a second eye.