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

fix NPE when concurrent delete #518

Merged
merged 5 commits into from
Aug 31, 2023
Merged

fix NPE when concurrent delete #518

merged 5 commits into from
Aug 31, 2023

Conversation

Yuqi-Du
Copy link
Contributor

@Yuqi-Du Yuqi-Du commented Aug 30, 2023

What this PR does:
findOneAndDelete with projection under concurrent situation.
For example, some requests read a successfully, but delete will fail because a has been deleted. In the retry process, JSON API will try to read a again, this time it read a null. Then when apply projection, there will be a NPE.

This issue will occur not with vectors, but also when doing findOneAndDelete with normal filter +projection.

The fix has passed the IT and also benchmark test.

Which issue(s) this PR fixes:
Fixes #517

Checklist

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

@Yuqi-Du Yuqi-Du requested a review from a team as a code owner August 30, 2023 16:00
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1683,6 +1679,114 @@ public void findOneAndReplace() {
}
}

@Nested
@Order(8)
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to be duplicate for FindOneAndDeleteIntegrationTest, but just with vector column.
I think I would not add these since it's more code to maintain and does not add much more verification.

@@ -321,4 +317,151 @@ public void checkMetrics() {
FindOneAndDeleteIntegrationTest.super.checkMetrics("FindOneAndDeleteCommand");
}
}

@Nested
class ConcurrentDelete {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good! If these failed (at least for some runs) without the fix, and pass (reliably) with the fix (couple of sample runs should be enough to prove), that's good.

@Yuqi-Du Yuqi-Du merged commit fdd8f5b into main Aug 31, 2023
@Yuqi-Du Yuqi-Du deleted the fix-concurrent-delete branch August 31, 2023 23:08
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.

findOneAndDelete by vector has NPE issue in concurrent situation
2 participants