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.

Sign up for GitHub

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

Jump to bottom

Inconsistent API behavior when deleting an actor reminder #6161

Closed
greenie-msft opened this issue Mar 30, 2023 · 4 comments · Fixed by #6387
Closed

Inconsistent API behavior when deleting an actor reminder #6161

greenie-msft opened this issue Mar 30, 2023 · 4 comments · Fixed by #6387
Assignees
@ItalyPaleAle
Labels
kind/bug Something isn't working
Milestone
v1.11

Comments

@greenie-msft
Copy link
Contributor

Activating Reminder partitioning causes api behavior to change on DELETE Reminder api.

If partitioning is enabled the delete actor reminder request returns a 500. If partitioning is disabled, the request returns a 20X.

https://docs.dapr.io/reference/api/actors_api/#delete-actor-reminder

Example Repository:
https://github.com/fabistb/dapr-actor-reminder-partitioning
@greenie-msft greenie-msft added the kind/bug Something isn't working label Mar 30, 2023
@artursouza artursouza added this to the v1.11 milestone Mar 30, 2023
@artursouza
Copy link
Member

I looked at the repo from Fabian. Does it mean that deleting a reminder that does not exist returns 500 with partitioning but 200 without partitioning?

@greenie-msft
Copy link
Contributor Author

@artursouza that is correct.

@ItalyPaleAle
Copy link
Contributor

@greenie-msft would you be able to try this with 1.10.5 once the next RC comes out? It does includes lots of changes in this space.

@ItalyPaleAle ItalyPaleAle self-assigned this May 22, 2023
ItalyPaleAle added a commit to ItalyPaleAle/dapr that referenced this issue May 22, 2023
@ItalyPaleAle
Fixed: deleting a reminder should be a no-op when the reminder doesn'…
48ff4d9 
…t exist

When deleting a reminder that doesn't exist already, the data in the reminder store was updated anyways, causing unnecessary storage operations.

More importantly, if reminder partitioning is enabled, this was causing Dapr to respond with an ISE code.

Fixes dapr#6161

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle mentioned this issue May 22, 2023
Merged
@ItalyPaleAle
Copy link
Contributor

Was able to reproduce. Fix in #6387

ItalyPaleAle added a commit to ItalyPaleAle/dapr that referenced this issue May 23, 2023
@ItalyPaleAle
Fixed: deleting a reminder should be a no-op when the reminder doesn'…
ed3a32a 
…t exist

When deleting a reminder that doesn't exist already, the data in the reminder store was updated anyways, causing unnecessary storage operations.

More importantly, if reminder partitioning is enabled, this was causing Dapr to respond with an ISE code.

Fixes dapr#6161

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
yaron2 pushed a commit that referenced this issue May 23, 2023
@ItalyPaleAle
Fixed: deleting a reminder should be a no-op when the reminder doesn'…
2864b79 
…t exist (#6387)

* Fixed: deleting a reminder should be a no-op when the reminder doesn't exist

When deleting a reminder that doesn't exist already, the data in the reminder store was updated anyways, causing unnecessary storage operations.

More importantly, if reminder partitioning is enabled, this was causing Dapr to respond with an ISE code.

Fixes #6161

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Add unit tests

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

---------

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
JoshVanL pushed a commit to JoshVanL/dapr that referenced this issue Jun 14, 2023
@ItalyPaleAle @JoshVanL
Fixed: deleting a reminder should be a no-op when the reminder doesn'…
3f0350b 
…t exist (dapr#6387)

* Fixed: deleting a reminder should be a no-op when the reminder doesn't exist

When deleting a reminder that doesn't exist already, the data in the reminder store was updated anyways, causing unnecessary storage operations.

More importantly, if reminder partitioning is enabled, this was causing Dapr to respond with an ISE code.

Fixes dapr#6161

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Add unit tests

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

---------

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ItalyPaleAle ItalyPaleAle

Labels
kind/bug Something isn't working
Projects
No open projects
1.11 Release Tracking Board
Status: Done
Milestone
v1.11
Development

Successfully merging a pull request may close this issue.

Fixed: deleting a reminder should be a no-op when the reminder doesn't exist ItalyPaleAle/dapr
3 participants
@ItalyPaleAle @artursouza @greenie-msft