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

[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

Merged
merged 19 commits into from
May 30, 2024

Conversation

charliecheng630
Copy link
Contributor

What changes were proposed in this pull request?

When dropping the XXX(Metalake/Catalog/Schema/Table/Fileset/Partition/Topic), if the XXX does not exist, then return false.

Why are the changes needed?

a clear drop behavior of return value and exception thrown.

Fix: #2434

Does this PR introduce any user-facing change?

No

How was this patch tested?

ITs and UTs

@jerryshao
Copy link
Contributor

@mchades would you please help to review this?

@mchades
Copy link
Contributor

mchades commented May 6, 2024

Are the changes in core module the same as #3100 ?

@charliecheng630
Copy link
Contributor Author

Are the changes in core module the same as #3100 ?

Yes.

@charliecheng630
Copy link
Contributor Author

Update. Please help to review again when you have time~

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

The JavaDoc for methods(dropXxx) like com.datastrato.gravitino.SupportsMetalakes#dropMetalake should be reviewed and revised if needed.

@charliecheng630
Copy link
Contributor Author

Updated. Please help to review again when you have time, thanks!

@charliecheng630
Copy link
Contributor Author

charliecheng630 commented May 9, 2024

@mchades Updated. Please help to review again when you have time, thanks!

@jerryshao
Copy link
Contributor

@charliecheng630 would you please rebase the code?

@jerryshao
Copy link
Contributor

@charliecheng630 can you please also confirm that if anything should be changed in the client side?

@charliecheng630 charliecheng630 marked this pull request as draft May 16, 2024 15:03
@charliecheng630 charliecheng630 force-pushed the #2434 branch 3 times, most recently from 5daefec to 549860c Compare May 21, 2024 12:42
@charliecheng630 charliecheng630 marked this pull request as ready for review May 21, 2024 15:38
@charliecheng630
Copy link
Contributor Author

@mchades Rebased & updated. Please help to review again when you have time, thanks!

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contributions!

@mchades mchades merged commit 992c787 into apache:main May 30, 2024
33 checks passed
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
…ists return false (apache#3222)

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

When dropping the
XXX(Metalake/Catalog/Schema/Table/Fileset/Partition/Topic), if the XXX
does not exist, then return false.

### Why are the changes needed?

a clear drop behavior of return value and exception thrown.

Fix: apache#2434 

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

ITs and UTs
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.

[Improvement] revisit all the dropXXX interfaces to have a consistent behavior
4 participants