Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw exception when asking for FirstWrite OCC without ETag #527

Open
amolenk opened this issue Jan 3, 2021 · 4 comments
Open

Throw exception when asking for FirstWrite OCC without ETag #527

amolenk opened this issue Jan 3, 2021 · 4 comments
Labels
kind/bug Something isn't working
Milestone

Comments

@amolenk
Copy link
Contributor

amolenk commented Jan 3, 2021

Describe the proposal

Currently, the SDK allows you to make the following call:

await daprClient.SaveStateAsync<Foo>(
    "storename",
    "key",
    new Foo(),
    new StateOptions
    {
        Concurrency = ConcurrencyMode.FirstWrite
    });

The problem with the call is that we're asking Dapr to do something impossible. FirstWrite OCC will only work if we send an ETag along with the request. SaveStateAsync will never send the ETag, for that you need to call TrySaveStateAsync.

The call will succeed but it won't use FirstWrite OCC. There are other situations where you can give Dapr state management a command/hint that cannot be fulfilled by the underlying state store, such as asking for strong consistency if the data store doesn't support it. However, the FirstWrite issue can be caught early at the SDK level.

I propose throwing an exception with a clear message explaining that you need ETag/TrySaveState to use FirstWrite.

@rynowak rynowak added the kind/bug Something isn't working label Jan 3, 2021
@rynowak
Copy link
Contributor

rynowak commented Jan 3, 2021

Thanks for the report. Agree that this doesn't make sense.

You said that the call succeeds - so in this case it's also accepted by the state store? I want to bubble this up to the Dapr API/runtime level as well. I think it's fine if the .NET SDK is as-strict with validation as the Dapr API, but it's not a good thing if we're more strict.

@rynowak
Copy link
Contributor

rynowak commented Jan 3, 2021

/cc @mukundansundar @yaron2

@amolenk
Copy link
Contributor Author

amolenk commented Jan 4, 2021

Yes, it's accepted by the state store. As long as the request doesn't contain an ETag, Dapr will always use last-write-wins.

The OCC mechanism also doesn't make any distinction between a blank and a missing ETag. This causes problems when inserting new data. I've added a comment to this existing issue: dapr/dapr#2320

I've also added a proposal to use HTTP 409 Conflict when ETags don't match instead of a generic HTTP 500. That way it will be much easier to distinguish ETag mismatches from other errors. dapr/dapr#2619

@rynowak
Copy link
Contributor

rynowak commented Jan 4, 2021

Thanks, that saves me the legwork 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants