[#5029] improvement(storage): Delete the related relations about the metadata object when it is deleted. #5426
Conversation
jerqi
force-pushed
the
ISSUE-5029
branch
7 times, most recently
from
eb7235d to
df6e04d
Compare
| public String softDeleteObjectRelsByCatalogId(@Param("catalogId") Long catalogId) { | ||
| return "UPDATE " | ||
| + SECURABLE_OBJECT_TABLE_NAME | ||
| + " sect SET deleted_at = (UNIX_TIMESTAMP() * 1000.0)" |
There was a problem hiding this comment.
Can we simply use this SQL as "
update SECURABLE_OBJECT_TABLE_NAME c set xxxxx
where sect.deleted_at = 0 and sec.type in ('catalog', 'schema'....) and exist (
.....
)
There was a problem hiding this comment.
sec.type are different for sub-sentences. deleted_atis ok.
| () -> | ||
| SessionUtils.doWithoutCommit( | ||
| SecurableObjectMapper.class, | ||
| mapper -> | ||
| mapper.softDeleteObjectRelsByMetadataObject( | ||
| catalogId, MetadataObject.Type.CATALOG.name())), | ||
| () -> | ||
| SessionUtils.doWithoutCommit( | ||
| TagMetadataObjectRelMapper.class, | ||
| mapper -> | ||
| mapper.softDeleteTagMetadataObjectRelsByMetadataObject( | ||
| catalogId, MetadataObject.Type.CATALOG.name())), | ||
| () -> | ||
| SessionUtils.doWithoutCommit( | ||
| CatalogMetaMapper.class, | ||
| mapper -> mapper.softDeleteCatalogMetasByCatalogId(catalogId))); |
There was a problem hiding this comment.
Does the deletion sequence of securableobject, tag and catalog matters in this transaction? why do you change the sequence of them?
There was a problem hiding this comment.
My fault. I shouldn't change the order.
| @@ -229,6 +231,31 @@ public boolean deleteTable(NameIdentifier identifier) { | |||
| if (deleteResult.get() > 0) { | |||
| TableColumnMetaService.getInstance().deleteColumnsByTableId(tableId); | |||
| } | |||
| }, | |||
| () -> { | |||
| if (deleteResult.get() > 0) { | |||
There was a problem hiding this comment.
This is a problem unrelated to this PR. please see L218, Since it uses doWithCommitAndFetchResult, so it's will use two transactions in the deleteTable if I'm not wrong.
There was a problem hiding this comment.
Yes, we should change this.
| @@ -231,7 +231,7 @@ public boolean deleteMetalake(NameIdentifier ident, boolean cascade) { | |||
| () -> | |||
| SessionUtils.doWithoutCommit( | |||
| SecurableObjectMapper.class, | |||
| mapper -> mapper.softDeleteRoleMetasByMetalakeId(metalakeId)), | |||
| mapper -> mapper.softDeleteSecurableObjectsByMetalakeId(metalakeId)), | |||
There was a problem hiding this comment.
Is this just a fix to a problem that existed before? I reviewed the code, L230 has the same code if it's not modified.
There was a problem hiding this comment.
One is deleting role meta data, another is deleting role's securable object metadata.
| SecurableObjectMapper.class, | ||
| mapper -> | ||
| mapper.softDeleteObjectRelsByMetadataObject( | ||
| tableId, MetadataObject.Type.TABLE.name())); | ||
| } | ||
| }, | ||
| () -> { | ||
| if (deleteResult.get() > 0) { | ||
| SessionUtils.doWithoutCommit( | ||
| TagMetadataObjectRelMapper.class, | ||
| mapper -> | ||
| mapper.softDeleteTagMetadataObjectRelsByMetadataObject( | ||
| tableId, MetadataObject.Type.TABLE.name())); | ||
| } | ||
| }, | ||
| () -> { | ||
| if (deleteResult.get() > 0) { | ||
| SessionUtils.doWithoutCommit( | ||
| TagMetadataObjectRelMapper.class, | ||
| mapper -> mapper.softDeleteTagMetadataObjectRelsByTableId(tableId)); | ||
| } |
There was a problem hiding this comment.
Can we merge this operation into one like
if (deleteResult.get() > 0) {
// one
SessionUtils.doWithoutCommit()
// two
SessionUtils.doWithoutCommit()
...
}
|
The code looks good to me. |
This reverts commit b4106dc.
What changes were proposed in this pull request?
Delete the related relations about the metadata object when it is deleted.
Why are the changes needed?
Fix: #5029
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add UTs.