dotnet/orleans

ADO Grain Storage Provider `IPersistentState<T>.RecordExists` returns `true` after calling `ClearStateAsync()` and doesn't actually delete the state from SQL

Opened this issue · 6 comments

ElanHasson commented

If you call use the ADO Storage Provider and write state, then clear state, RecordExists will always return true.

Steps to Repro:

  1. Configure ADO Grain Storage
  2. Activate a grain with a unique ID and NO state record in SQL.
  3. state.RecordExists will be false
  4. call IPersistentState<T>.WriteStateAsync()
  5. Observe record in SQL with State data
  6. call another grain method on same ID and call IPersistentState<T>.ClearStateAsync()
  7. Observe record in SQL with State data and state.RecordExists will still be true

Expected Behavior:

Ideally the row in sql would be deleted.

If not feasible, then if Payload column is null then RecordExist should be false.

ReubenBond commented

Linking the original RecordExists PR #6580

  • 👍1
ElanHasson commented

If we change the behavior, it'd be considered a breaking change IMO.

ReubenBond commented

Based on the PR I linked, it should be getting set to false after ClearStateAsync - do you know why it's not?

ElanHasson commented

Because the record exists it'll never be false after a state exists and is cleared. I believe it should check if a record exists OR all three payload columns are null to determine if it doesn't exist.

This is due to ClearStateAsync not deleting records for ADO.

https://github.com/dotnet/orleans/blob/main/src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/AdoNetGrainStorage.cs#L313-L322

As for it being false after you clear the state, that will only last until the next grain activation: https://github.com/dotnet/orleans/blob/main/src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/AdoNetGrainStorage.cs#L202

ElanHasson commented

I need this in 3.x, so I think the move may be to make this an opt-in change in behavior to actually delete the row.

siloHostBuilder.UseAdoNetClustering(options =>
{
    options.Invariant = invariant;
    options.ConnectionString = connectionString;
    options.UseLogicalClearState = false; // in 3.x, default would be true to maintain existing behavior, in 4.0, default would be false to make this provider consistent with expectations.
});

Thoughts on this @ReubenBond ?

  • ❤️1
ElanHasson commented

Should probably be DeleteOnClear to match other providers.
image

  • ❤️2