[#2697] improvement(core): Support different entity with the same name under a namespace #2700
Conversation
|
I think the key thing here is to make this change backward compatible, otherwise there's no way for users to upgrade. |
Yeah, that is why I'm hesitant about it. |
| @@ -199,72 +189,15 @@ public <E extends Entity & HasIdentifier> E update( | |||
|
|
|||
| // Update the name mapping | |||
| nameMappingService.updateName( | |||
| generateKeyForMapping(ident), generateKeyForMapping(updatedE.nameIdentifier())); | |||
| generateKeyForMapping(ident, entityType, nameMappingService), | |||
| generateKeyForMapping(updatedE.nameIdentifier(), entityType, nameMappingService)); | |||
There was a problem hiding this comment.
Only changing here is enough, do you need to change put/get also?
There was a problem hiding this comment.
all have been changed through method concatIdAndName and put/get/update will use this method to generate key format.
I have updated the code and added support for backward compatibility. Please take a look if you have time. Thanks. |
| prefixes.add(replacePrefixTypeInfo(encode, FILESET.getShortName())); | ||
| break; | ||
| case TABLE: | ||
| case FILESET: |
There was a problem hiding this comment.
Should you also add USER and GROUP here?
There was a problem hiding this comment.
If we add USER and GROUP here, USER and GROUP will block the non-cascade deletion of metalake.
What changes were proposed in this pull request?
Support store entities with the same name under a namespace.
Why are the changes needed?
It's a necessary feature for Gravitino.
Fix: #2697
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
UTs