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

[#4968] improvement(api): Unify the modification behavior of the comment field #5121

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

koonchen
Copy link
Contributor

@koonchen koonchen commented Oct 12, 2024

What changes were proposed in this pull request?

  • deprecated the removeComment change
  • the new comment of updateComment supports null and empty string

Why are the changes needed?

Fix: #4968

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Pass existing tests

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 PR title should be:
[#4968] improvement(api): Unify the modification behavior of the comment field

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 of API updateComment should also be updated

@koonchen koonchen changed the title [#4968] Unify the modification behavior of the comment field (#4968) [#4968] improvement(api): Unify the modification behavior of the comment field Oct 12, 2024
@koonchen koonchen force-pushed the issue#4968 branch 2 times, most recently from 9c3c482 to 06681cd Compare October 12, 2024 08:55
@mchades
Copy link
Contributor

mchades commented Oct 16, 2024

@koonchen could you plz update the code to make the CI pass?

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.

You should also add tests to cover this change

@koonchen
Copy link
Contributor Author

@koonchen could you plz update the code to make the CI pass?

OK, I'm a little bit busy these days, I'll take some time this week to deal with it

@koonchen koonchen force-pushed the issue#4968 branch 2 times, most recently from a664656 to 3fae0cc Compare October 20, 2024 06:32
@koonchen
Copy link
Contributor Author

@mchades PTAL

@mchades
Copy link
Contributor

mchades commented Oct 25, 2024

@koonchen could you plz resolve the comments?

@koonchen
Copy link
Contributor Author

ok, I handle it on the weekend

@koonchen koonchen force-pushed the issue#4968 branch 2 times, most recently from fa2fd28 to c7446a1 Compare October 26, 2024 02:27
@koonchen
Copy link
Contributor Author

@mchades I have been traveling these days. Thank you for your help. You are a good mentor PTAL

@mchades
Copy link
Contributor

mchades commented Oct 26, 2024

plz fix the CI

@mchades
Copy link
Contributor

mchades commented Oct 26, 2024

I noticed that you only modified the behavior of Fileset. The Metalake, Catalog, Table, and Topic also need to be changed.

@mchades
Copy link
Contributor

mchades commented Oct 26, 2024

I want to include this change in 0.7, in order to catch up with the release of 0.7.

If you are pressed for time and do not mind, may I take over next week to finish the rest of it based on this PR?

@koonchen
Copy link
Contributor Author

@mchades Thanks, I can still give it a try tomorrow, Catalog Metalake Table Tag Topic, has been modified to allow null or empty string for updateComment operation, are you saying they also need an explicit removeComment method?

@mchades
Copy link
Contributor

mchades commented Oct 26, 2024

are you saying they also need an explicit removeComment method?

Sorry, I made a mistake. I will review it as soon as possible

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.

you should also add integration tests in CatalogIT, MetalakeIT and CatalogKafkaIT

clients/client-python/gravitino/api/fileset_change.py Outdated Show resolved Hide resolved
@koonchen koonchen force-pushed the issue#4968 branch 5 times, most recently from 9ed1ecd to b23de8f Compare October 27, 2024 14:19
@koonchen
Copy link
Contributor Author

@mchades PTAL

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 added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 28, 2024
@mchades mchades merged commit c5ecc85 into apache:main Oct 28, 2024
26 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 28, 2024
…ent field (#5121)

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

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: #4968 

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

N/A

### How was this patch tested?

Pass existing tests

Co-authored-by: chenzeping.ricco <[email protected]>
mplmoknijb pushed a commit to mplmoknijb/gravitino that referenced this pull request Nov 6, 2024
…e comment field (apache#5121)

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

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: apache#4968 

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

N/A

### How was this patch tested?

Pass existing tests

Co-authored-by: chenzeping.ricco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-0.7 Automatically cherry-pick commit to branch-0.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Unify the modification behavior of the comment field.
2 participants