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

[Improvement] Make property cloud.name and cloud.region-code mutable #4999

Closed
mchades opened this issue Sep 24, 2024 · 4 comments · Fixed by #5025
Closed

[Improvement] Make property cloud.name and cloud.region-code mutable #4999

mchades opened this issue Sep 24, 2024 · 4 comments · Fixed by #5025
Assignees
@LindaSummer
Labels
0.7.0 Release v0.7.0 good first issue Good for newcomers improvement Improvements on everything

Comments

@mchades
Copy link
Contributor

mchades commented Sep 24, 2024

What would you like to be improved?

Because there is a possibility that the creation was filled in incorrectly, or that the region changed as the URL of the catalog changed, we should allow modification of these two properties

How should we improve?

Make property cloud.name and cloud.region-code mutable
@mchades mchades added improvement Improvements on everything good first issue Good for newcomers labels Sep 24, 2024
@LindaSummer
Copy link
Contributor

Hi @mchades ,

I want to follow up this issue.

Can it be assigned to me?

Best Regards,
Edward

@mchades
Copy link
Contributor Author

mchades commented Sep 24, 2024

Sure, assigned. @LindaSummer

@LindaSummer
Copy link
Contributor

Hi @mchades ,

Sorry to bother you.

I'm not sure related test case should be put in which test class.

Could you give me some advice?

Best Regards,
Edward

@mchades
Copy link
Contributor Author

mchades commented Sep 24, 2024

You can modify the tests:

gravitino/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalogOperations.java

Lines 105 to 106 in a89fe92

Assertions.assertFalse(propertyEntryMap.get(CLOUD_NAME).isRequired());
Assertions.assertFalse(propertyEntryMap.get(CLOUD_REGION_CODE).isRequired());

and add integration tests to here:

gravitino/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java

Line 45 in a89fe92

public class CatalogIT extends AbstractIT {

@LindaSummer LindaSummer mentioned this issue Sep 26, 2024
Merged
@mchades mchades added the 0.7.0 Release v0.7.0 label Sep 26, 2024
yuqi1129 pushed a commit that referenced this issue Sep 26, 2024
@LindaSummer
[#4999] feat(catalog): make cloud.name and cloud.region-code muta…
c64ded6 
…ble. (#5025)

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

Make catalog property `cloud.name` and `cloud.region-code` mutable.

### Why are the changes needed?

As #4999 mentioned, the `cloud.name` and `cloud.region-code` may change
after first creation.

Fix: #4999 

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

No.

### How was this patch tested?

Related integration tests have been added in this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LindaSummer LindaSummer

Labels
0.7.0 Release v0.7.0 good first issue Good for newcomers improvement Improvements on everything
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#4999] feat(catalog): make cloud.name and cloud.region-code mutable. LindaSummer/gravitino
2 participants
@LindaSummer @mchades