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

[#3092] fix(relational-entity-store): Return false when deleting throw a NoSuchEntityException #3100

Merged
merged 5 commits into from
May 9, 2024
Merged

[#3092] fix(relational-entity-store): Return false when deleting throw a NoSuchEntityException #3100

merged 5 commits into from
May 9, 2024

Conversation

xloya
Copy link
Contributor

@xloya xloya commented Apr 22, 2024 •
edited
Loading

What changes were proposed in this pull request?

When Relational Entity Store deleteing an entity and throwing a NoSuchEntityException, return false finally. It's consistent with KV Store.

Why are the changes needed?

Fix: #3092

How was this patch tested?

Add some duplicate deleted uts.

@xloya
Copy link
Contributor Author

xloya commented Apr 22, 2024

cc @jerryshao @yuqi1129

@xloya
Copy link
Contributor Author

xloya commented Apr 22, 2024 •
edited
Loading

Another thing to confirm is that in the Relational Entity Store, there are two situations where NoSuchEntityException will be returned when deleting an entity:
1). If it is found that the parent entity does not exist, throw NoSuchEntityException;
2). If it is found that the entity that needs to be operated does not exist, throw NoSuchEntityException;

I'm not sure returning false for both cases is consistent with the KV Store.

@xloya xloya changed the title [#3092] improvement(relational-store): Return false when deleting throw a NoSuchEntityException [#3092] improvement(relational-entity-store): Return false when deleting throw a NoSuchEntityException Apr 22, 2024
@jerryshao
Copy link
Contributor

@yuqi1129 can you please confirm this?

@mchades mchades mentioned this pull request May 6, 2024
Merged
@yuqi1129
Copy link
Contributor

yuqi1129 commented May 6, 2024

@yuqi1129 can you please confirm this?

Generally, I'm okay with the changes. The logic is consistent with the KV store. @jerryshao WDYT

@jerryshao
Copy link
Contributor

I'm fine if the semantics are consistent. @yuqi1129 @qqqttt123 please review the detailed implementation to see if it is OK or not.

@jerryshao jerryshao requested review from yuqi1129 and qqqttt123 May 9, 2024 03:37
@qqqttt123
Copy link
Contributor

Maybe this is a fix instead of an improvement.

@xloya xloya changed the title [#3092] improvement(relational-entity-store): Return false when deleting throw a NoSuchEntityException [#3092] fix(relational-entity-store): Return false when deleting throw a NoSuchEntityException May 9, 2024
@yuqi1129 yuqi1129 closed this May 9, 2024
@yuqi1129 yuqi1129 reopened this May 9, 2024
@yuqi1129 yuqi1129 added need backport Issues that need to backport to another branch branch-0.5 labels May 9, 2024
yuqi1129
yuqi1129 approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuqi1129 yuqi1129 merged commit c740b13 into apache:main May 9, 2024
35 of 47 checks passed
github-actions bot pushed a commit that referenced this pull request May 9, 2024
@xloya @web-flow
[#3092] fix(relational-entity-store): Return false when deleting thro…
098861e 
…w a `NoSuchEntityException` (#3100)

### What changes were proposed in this pull request?

When Relational Entity Store deleteing an entity and throwing a
NoSuchEntityException, return false finally. It's consistent with KV
Store.

### Why are the changes needed?

Fix: #3092 

### How was this patch tested?

Add some duplicate deleted uts.

---------

Co-authored-by: xiaojiebao <[email protected]>
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
@xloya @diqiu50
[apache#3092] fix(relational-entity-store): Return false when deletin…
ee02221 
…g throw a `NoSuchEntityException` (apache#3100)

### What changes were proposed in this pull request?

When Relational Entity Store deleteing an entity and throwing a
NoSuchEntityException, return false finally. It's consistent with KV
Store.

### Why are the changes needed?

Fix: apache#3092 

### How was this patch tested?

Add some duplicate deleted uts.

---------

Co-authored-by: xiaojiebao <[email protected]>
@xloya xloya deleted the issue-3092 branch June 20, 2024 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqqttt123 qqqttt123 qqqttt123 left review comments

@yuqi1129 yuqi1129 yuqi1129 approved these changes

Assignees

@xloya xloya

Labels
branch-0.5 need backport Issues that need to backport to another branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] The semantics of delete operation for relational storage should be aligned with kv storage
4 participants
@xloya @jerryshao @yuqi1129 @qqqttt123