[#4018] feat(core): Add tag management logic for Tag System (Part 1) #4019

jerryshao · 2024-07-02T06:27:05Z

What changes were proposed in this pull request?

This PR tracks the work of adding the core logics for tag management.

Why are the changes needed?

This is a part of work for adding tag support in Gravitino.

Fix: #4018

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UTs added.

core/src/main/java/com/datastrato/gravitino/meta/TagEntity.java

yuqi1129 · 2024-07-02T07:54:32Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java

+  public void insertTag(TagEntity tagEntity, boolean overwritten) throws IOException {
+    Namespace ns = tagEntity.namespace();
+    String metalakeName = ns.level(0);
+    AtomicReference<Long> metalakeId = new AtomicReference<>(null);


Is there any thread-safe problem here? If not, I think Long[] metalakeId = new Long[] {metalakeId} may be enough.

It is just a preference, I think both are fine.

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java

core/src/main/java/com/datastrato/gravitino/tag/TagManager.java

qqqttt123 · 2024-07-02T09:24:17Z

core/src/main/java/com/datastrato/gravitino/storage/relational/mapper/TagMetaMapper.java

+          + MetalakeMetaMapper.TABLE_NAME
+          + " mm on tm.metalake_id = mm.metalake_id"
+          + " WHERE mm.metalake_name = #{metalakeName} AND tm.deleted_at = 0")
+  List<TagPO> listTagPOsByMetalake(@Param("metalakeName") String metalakeName);


If metalake doesn't exist, it will return empty collection instead of throwing NoSuchMetalakeException.

I have already checked this outside in TagManager.

Even in the other implementation you will still get an empty collection if it happens that after the check and before the select the metalake is deleted.

Do we need to put the check into one transaction in later pull request?

I would hesitate to do so, it is overkill to put the check in one transaction. Also, I was thinking about the behavior of the list, for example, if we're going to list the entities under a non-existent namespace, should it return an empty list or throw a namespace not exist exception? Both are reasonable, I need to think a bit about this.

core/src/main/java/com/datastrato/gravitino/storage/relational/mapper/TagMetaMapper.java

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java

jerryshao · 2024-07-05T03:58:35Z

@xloya @yuqi1129 @lw-yang can you please help me to review this? I use the transactions here for multiple step queries, I'm not so sure if it is correct or not, also I'm very sure we can really keep the consistency, or we have to introduce the lock here.

jerryshao · 2024-07-05T04:02:28Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java

+
+    try {
+      SessionUtils.doMultipleWithCommit(
+          () -> metalakeId.set(MetalakeMetaService.getInstance().getMetalakeIdByName(metalakeName)),


I was thinking that select... cannot block the delete operation, so we need to have a metalake lock here to avoid user deleting the metalake while we're querying it.

It seems to we need use select xxx for update to obtain the write lock here, I'm not very it will cause dead lock or other problem or not.

One choice is to use select...for update.

I was planning to avoid using tag lock or tree lock, instead using transactions to keep consistency, but seems it is not achievable, still we have to introduce lock here.

According to the current design, I think we'd better add locks in the upper layer to avoid duplication of internal locks and database locks.

Since there is a upper lock mechanism like tree lock, using it is a reasonable solution.

xloya · 2024-07-05T06:17:10Z

...main/java/com/datastrato/gravitino/storage/relational/mapper/TagMetadataObjectRelMapper.java

+          + " + EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3)) / 1000"
+          + " WHERE tmo.tag_id IN (SELECT tm.tag_id FROM "
+          + TagMetaMapper.TAG_TABLE_NAME
+          + " tm WHERE tm.metalake_id IN (SELECT mm.metalake_id FROM "


How about using JOIN in the sub queries here? As far as I know, the performance of the IN condition will be poor when the amount of data is large, so we'd better try to reduce the use of IN.

H2 doesn't support JOIN, so I use subquery here.

...main/java/com/datastrato/gravitino/storage/relational/mapper/TagMetadataObjectRelMapper.java

xloya · 2024-07-05T06:28:53Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java

+
+    try {
+      SessionUtils.doMultipleWithCommit(
+          () -> metalakeId.set(MetalakeMetaService.getInstance().getMetalakeIdByName(metalakeName)),


According to the current design, I think we'd better add locks in the upper layer to avoid duplication of internal locks and database locks.

xloya · 2024-07-05T06:31:20Z

scripts/mysql/schema-0.6.0-mysql.sql

+    UNIQUE KEY `uk_ti_mi_del` (`tag_id`, `metadata_object_id`, `deleted_at`),
+    KEY `idx_tid` (`tag_id`),
+    KEY `idx_mid` (`metadata_object_id`)


I don't see any query using this condition in the current SQL, so it seems that this index is unnecessary now. But we can also reserve this index if we are sure that we will need to query based on this condition in the future.

It will be used in the next PR.

jerryshao · 2024-07-05T09:11:27Z

@yuqi1129 @xloya @lw-yang please help to review again.

yuqi1129 · 2024-07-08T11:49:06Z

Overall, LGTM is good except for a minor issue. Please let me know if it has been resolved or if there is any doubt.

core/src/main/java/com/datastrato/gravitino/tag/TagManager.java

yuqi1129

LGTM

yuqi1129 · 2024-07-08T14:29:41Z

@qqqttt123 @lw-yang
Do you have any additional comments on the matter?

lw-yang · 2024-07-09T01:07:14Z

LGTM

xloya · 2024-07-09T01:35:19Z

LGTM

jerryshao requested review from shaofengshi, yuqi1129 and mchades July 2, 2024 06:27

jerryshao self-assigned this Jul 2, 2024

yuqi1129 reviewed Jul 2, 2024

View reviewed changes

jerryshao changed the title ~~[#4018] feat(core): Add tag management logic for Tag System~~ [#4018] feat(core): Add tag management logic for Tag System (Part 1) Jul 2, 2024

qqqttt123 reviewed Jul 2, 2024

View reviewed changes

lw-yang reviewed Jul 2, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/storage/relational/mapper/TagMetaMapper.java Outdated Show resolved Hide resolved

jerryshao closed this Jul 2, 2024

jerryshao reopened this Jul 2, 2024

jerryshao force-pushed the issue-4018 branch from db51559 to 6505693 Compare July 3, 2024 08:59

jerryshao mentioned this pull request Jul 3, 2024

[Improvement] Improve the current SQL of JDBCBackend #4024

Open

jerryshao requested review from yuqi1129, lw-yang and qqqttt123 July 4, 2024 05:59

yuqi1129 reviewed Jul 4, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/storage/relational/service/TagMetaService.java Outdated Show resolved Hide resolved

jerryshao commented Jul 5, 2024

View reviewed changes

xloya reviewed Jul 5, 2024

View reviewed changes

yuqi1129 reviewed Jul 8, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/tag/TagManager.java Outdated Show resolved Hide resolved

jerryshao added 8 commits July 8, 2024 19:55

Support relational storage for tag system

14eecaa

Continue to add more UTs

1ada264

slight improvement

22f9182

Fix IT issue

f236aa5

Add blank line

2e33e5a

Fix IT failure

c888d3b

Address the comments

3d120d7

Address the comments

5d1667e

jerryshao added 7 commits July 8, 2024 19:55

Change to use ASF header and fix comment

454c244

Fix query problem

8c0ec85

Address the comment

f28d734

Refactor the code

ec59eec

Address the comment

48e160d

Address the comment

d674a96

Revert disabled tests

93c1d78

jerryshao force-pushed the issue-4018 branch from 01181ca to 93c1d78 Compare July 8, 2024 12:04

yuqi1129 approved these changes Jul 8, 2024

View reviewed changes

jerryshao merged commit 178eb37 into apache:main Jul 9, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4018] feat(core): Add tag management logic for Tag System (Part 1) #4019

[#4018] feat(core): Add tag management logic for Tag System (Part 1) #4019

jerryshao commented Jul 2, 2024

yuqi1129 Jul 2, 2024

jerryshao Jul 2, 2024

qqqttt123 Jul 2, 2024

jerryshao Jul 2, 2024

qqqttt123 Jul 2, 2024 •

edited

Loading

jerryshao Jul 3, 2024

jerryshao commented Jul 5, 2024

jerryshao Jul 5, 2024

yuqi1129 Jul 5, 2024

jerryshao Jul 5, 2024

jerryshao Jul 5, 2024

xloya Jul 5, 2024

yuqi1129 Jul 5, 2024

xloya Jul 5, 2024

jerryshao Jul 5, 2024

xloya Jul 5, 2024

xloya Jul 5, 2024 •

edited

Loading

jerryshao Jul 5, 2024

jerryshao commented Jul 5, 2024

yuqi1129 commented Jul 8, 2024

yuqi1129 left a comment

yuqi1129 commented Jul 8, 2024

lw-yang commented Jul 9, 2024

xloya commented Jul 9, 2024

[#4018] feat(core): Add tag management logic for Tag System (Part 1) #4019

[#4018] feat(core): Add tag management logic for Tag System (Part 1) #4019

Conversation

jerryshao commented Jul 2, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xloya Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Jul 5, 2024

yuqi1129 commented Jul 8, 2024

yuqi1129 left a comment

Choose a reason for hiding this comment

yuqi1129 commented Jul 8, 2024

lw-yang commented Jul 9, 2024

xloya commented Jul 9, 2024

qqqttt123 Jul 2, 2024 •

edited

Loading

xloya Jul 5, 2024 •

edited

Loading