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

Delete - LWT failure retries #218

Merged
merged 16 commits into from
Mar 7, 2023
Merged

Delete - LWT failure retries #218

merged 16 commits into from
Mar 7, 2023

Conversation

maheshrajamani
Copy link
Contributor

@maheshrajamani maheshrajamani commented Mar 2, 2023

What this PR does:
Delete - Handling LWT failure retries
Which issue(s) this PR fixes:
Fixes #217

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

#	src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/DeleteOperationTest.java
#	src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/DeleteManyCommandResolverTest.java
#	src/test/java/io/stargate/sgv2/jsonapi/service/resolver/model/impl/DeleteOneCommandResolverTest.java
@maheshrajamani maheshrajamani marked this pull request as ready for review March 2, 2023 21:11
@maheshrajamani maheshrajamani requested a review from a team as a code owner March 2, 2023 21:11
@maheshrajamani
Copy link
Contributor Author

maheshrajamani commented Mar 2, 2023

@ivansenic @tatu-at-datastax I have done IT for this PR. Not sure how to generate LWT failure for IT test case.
Note: The transaction fails with first failure and throws errors. Error handling to return both count and error message will be handled as part of #214

@maheshrajamani
Copy link
Contributor Author

/gcbrun

Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, plus missing integration test.. Should be done with multiple update threads and one delete thread.. Or one update and one delete thread, but then the test must run many times to see both can succeed..

@maheshrajamani
Copy link
Contributor Author

Should be done with multiple update threads and one delete thread.. Or one update and one delete thread, but then the test must run many times to see both can succeed..

@ivansenic None of these guarantee delete retry logic is executed. Can't assert for the use case with above testing.

@maheshrajamani maheshrajamani requested a review from ivansenic March 6, 2023 15:45
@tatu-at-datastax
Copy link
Contributor

Should be done with multiple update threads and one delete thread.. Or one update and one delete thread, but then the test must run many times to see both can succeed..

@ivansenic None of these guarantee delete retry logic is executed. Can't assert for the use case with above testing.

We discussed this with Aaron, Mahesh and myself and considered it very difficult if not impossible to have a reproducible reliable test to verify this, as an IT. But that later on it could be tested with load test. Unit test can verify other aspects.

Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, but there are still some issue that should be fixed:

  • simplify reactive sequence
  • assert JWT failure in the JsonApiException
  • etc

@ivansenic
Copy link
Contributor

ivansenic commented Mar 7, 2023

Should be done with multiple update threads and one delete thread.. Or one update and one delete thread, but then the test must run many times to see both can succeed..

@ivansenic None of these guarantee delete retry logic is executed. Can't assert for the use case with above testing.

We discussed this with Aaron, Mahesh and myself and considered it very difficult if not impossible to have a reproducible reliable test to verify this, as an IT. But that later on it could be tested with load test. Unit test can verify other aspects.

But verify what? I mean it's fairly easy to do an update and delete in parallel. They both have to succeed. Run this N amount of times in a @RepeatableTest, and you get the assurance that retries work. Yes I agree, it's not directly assuring that retry was executed, but if you write this test and apply to the current state, it would fail actually. So if you ask me that's pretty good, way better than not having it.

@maheshrajamani cc

@maheshrajamani
Copy link
Contributor Author

But verify what? I mean it's fairly easy to do an update and delete in parallel. They both have to succeed. Run this N amount of times in a @RepeatableTest, and you get the assurance that retries work. Yes I agree, it's not directly assuring that retry was executed, but if you write this test and apply to the current state, it would fail actually. So if you ask me that's pretty good, way better than not having it.

@maheshrajamani maheshrajamani merged commit eecac8f into main Mar 7, 2023
@maheshrajamani maheshrajamani deleted the delete-lwt-retry branch March 7, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete - LWT transaction failure retries
3 participants