[#3348] feat(core,server): Add the list operation of the user #4055

jerqi · 2024-07-03T08:53:43Z

What changes were proposed in this pull request?

Add the list operation of the user

Why are the changes needed?

Fix: #3348

Does this PR introduce any user-facing change?

I will add the document later.

How was this patch tested?

Add the new ut.

lw-yang · 2024-07-03T08:59:23Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

+  public String[] listUsers(String metalake) throws NoSuchMetalakeException {
+    return doWithNonAdminLock(() -> userGroupManager.listUsers(metalake));
+  }
+
+  public User[] listUsersInfo(String metalake) throws NoSuchMetalakeException {
+    return doWithNonAdminLock(() -> userGroupManager.listUsersInfo(metalake));
+  }


I think listUsers -> listUserNames, listUsersInfo -> listUsers would be more appropriate

jerqi · 2024-07-04T03:02:38Z

@lw-yang Could you take an another look?

lw-yang · 2024-07-04T03:58:45Z

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

+    for (UserPO userPO : userPOs) {
+      List<RolePO> rolePOs = RoleMetaService.getInstance().listRolesByUserId(userPO.getUserId());
+      userEntities.add(POConverters.fromUserPO(userPO, rolePOs, namespace));
+    }


the num of users > 1w is common situation, this may execute for a long time, maybe we should consider using cache in listUsers

cc @yuqi1129 Maybe we can have a common cache layer for this.

I think it is better to use a paging query.

jerryshao · 2024-07-12T08:53:05Z

@jerqi can you please update the PR.

jerqi · 2024-07-16T07:49:30Z

@lw-yang @jerryshao Do you have any other suggestion?

jerryshao · 2024-07-17T12:55:38Z

core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java

+      LOG.error("Listing user under metalake {} failed due to storage issues", metalake, ioe);
+      throw new RuntimeException(ioe);
+    }
+  }


I think you need to use tree lock here.

listUserNames can be implemented based on listUsers, don't need to duplicate the codes.

I think you need to use tree lock here.

I have used tree lock in the UserOperations.

listUserNames can be implemented based on listUsers, don't need to duplicate the codes.

OK, I have refactored the code.

jerryshao · 2024-07-17T12:59:53Z

core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java

+            userPOs.addAll(
+                SessionUtils.doWithoutCommitAndFetchResult(
+                    UserMetaMapper.class,
+                    mapper -> mapper.listUserPOsByMetalakeId(metalakeId.get()))));


I think we don't have to use transaction here, simply because we don't use select for update, so the transaction is not useful and will lead to dead lock.

I modified. But it shouldn't bring dead lock using the transaction.

jerryshao · 2024-07-18T07:34:50Z

common/src/main/java/org/apache/gravitino/dto/responses/NameListResponse.java

+@Getter
+@ToString
+@EqualsAndHashCode(callSuper = true)
+public class NameListResponse extends BaseResponse {


You should implement validate() method.

CatalogListResponse doesn't implement validate, too. I created an issue #4197 to track it.

jerryshao · 2024-07-18T07:35:05Z

common/src/main/java/org/apache/gravitino/dto/responses/UserListResponse.java

+    super(0);
+    this.users = null;
+  }
+}


jerryshao · 2024-07-18T07:40:50Z

core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java

+    for (UserPO userPO : userPOs) {
+      List<RolePO> rolePOs = RoleMetaService.getInstance().listRolesByUserId(userPO.getUserId());
+      userEntities.add(POConverters.fromUserPO(userPO, rolePOs, namespace));
+    }


Do you really want to return all the roles for all the users? This will be extremely cost, is that really the frontend needs?

Now, we adopts the lazily loading information about users/roles.

jerryshao · 2024-07-18T07:42:36Z

core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java

+    } catch (Throwable t) {
+      throw t;
+    } finally {
+      SqlSessions.closeSqlSession();


Why do you need to add this?

jerqi · 2024-09-09T03:27:26Z

After discussing with @shaofengshi offline, we need the ability to list all the role information of the user. It will be convenient for users. I have supported the user lazily loading. So if we only want to load the user names, we don't need to load unnecessary information.
In the future, we have many users, we can support to page. It's an improvement. We can support it in an another pull request.

xunliu · 2024-09-13T03:38:19Z

Query all data at once or cache buffering in the service also has the problem of dirty data. I think paged queries are better.
@jerryshao @jerqi What do you think?

jerryshao · 2024-09-14T00:17:35Z

core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java

+    try {
+      Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake);
+      return store.list(namespace, UserEntity.class, Entity.EntityType.USER).stream()
+          .map(entity -> (User) entity)


Why do we need this line?

So you only get the user list here, right? Will you also fetch the roles for each user, I saw that User and UserDTO have roles fields, will this be empty?

Yes, I only get the user list here. I won't fetch roles unless they are needed. Now the roles is empty for every user.

You can see the pull request. #4690
UserEntity roles field don't be fetched directly. We will put a supplier method field instead. So we don't fetch the values directly if we only need the names.
If we need all the information of entities, we can also have the ability to put them. You can see the list details. Because the front side need the all the users information, so I provide the interface.

I mean here why do we need to do a type conversion entity -> (User) entity?

Sorry, it's unnecessary. I ever designed other interface listEntitiesByRelation. It need me to use type cast. For this place, we don't need. I have removed it.

jerryshao · 2024-09-14T00:24:20Z

core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java

+        + " mt ON ut.metalake_id = mt.metalake_id"
+        + " WHERE mt.metalake_name = #{metalakeName}"
+        + " AND ut.deleted_at = 0 AND mt.deleted_at = 0";
+  }


This SQL will not throw an exception when metalake is not existed, it will only return an empty list. In your definition, you have NoSuchMetalakeException defined, to throw an such exception, you have to add one more check to see if metalake is existed or not.

Added the checkMetalakeExists.

jerryshao · 2024-09-14T00:25:27Z

server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java

+    ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class);
+    Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse2.getCode());
+    Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse2.getType());
+  }


I think you also need to add integration tests for this.

I have added IT in the AccessControlId.java.

jerryshao · 2024-09-23T05:57:34Z

Please fix the IT issue here.

yuqi1129 · 2024-09-23T06:11:21Z

@xunliu , @yuqi1129 you wanner another look?

Sure, I will review it today.

jerqi · 2024-09-23T06:48:07Z

Please fix the IT issue here.

I have fixed the issue. Waiting for CI.

core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java

core/src/main/java/org/apache/gravitino/EntityStore.java

xunliu · 2024-09-23T10:05:34Z

core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java

+ * data from multiple joined tables. The PO won't be written to database. So we don't need the inner
+ * class Builder.
+ */
+public class CombinedUserPO extends UserPO {


I think CombinedUserPO may not be a good name, But I haven't a good suggestion.
CombinedUserPO is more like a combined multiple-user.

How about ExtendUserPO?

xunliu · 2024-09-23T10:26:27Z

...ain/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java

+    for (SupportsSkippingFields<T> method : methods) {
+      if (skippingFields.containsAll(method.supportsSkippingFields())) {
+        return method.execute();
+      }
+    }


This is a double negative logic, let me confused.
Why select SQL based on skip fields instead of required fields?

Because the number of required fields may be very large, and it's hard to maintain when we add a new field.

Wouldn't it be clearer to use an enumeration?

From my opinion, it can't improve the code readability to use enumeration. It seems better to use fields instead of enumeration. Because it's more flexible and robust than enumeration.

xunliu · 2024-09-23T10:29:00Z

core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java

+    public List<Field> supportsSkippingFields() {
+      return Lists.newArrayList(UserEntity.ROLE_IDS, UserEntity.ROLE_NAMES);
+    }
+
+    @Override
+    public List<UserEntity> execute() {
+      List<UserPO> userPOs =
+          SessionUtils.getWithoutCommit(
+              UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName));
+      return userPOs.stream()
+          .map(
+              po ->
+                  POConverters.fromUserPO(
+                      po,
+                      Collections.emptyList(),
+                      AuthorizationUtils.ofUserNamespace(metalakeName)))
+          .collect(Collectors.toList());
+    }
+  }


SkippingFields and execution are just a mapping and are not directly related, e.g. execution needs to use the values in SkippingFields.

It's hard to use skippingFields to generate the SQL. So this just uses mapping.

yuqi1129 · 2024-09-23T11:13:43Z

core/src/main/java/org/apache/gravitino/EntityStore.java

+   * @param type the detailed type of the entity
+   * @param entityType the general type of the entity
+   * @param skippingFields Some fields may have a relatively high acquisition cost, EntityStore
+   *     provide an optional setting to avoid fetching these high-cost fields to improve the


yuqi1129 · 2024-09-23T11:17:16Z

...ain/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java

+import org.apache.gravitino.Field;
+
+/**
+ * This class is the collection wrapper of SupportsSkippingFields handler. The class will contains


contains-> contain

yuqi1129 · 2024-09-23T11:24:19Z

core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java

+   *
+   * @return The fields which could be skipped.
+   */
+  List<Field> supportsSkippingFields();


supportsSkippingFields -> allSkppingFields or totalSkipFields.

xunliu

LGTM
@jerryshao you wanner another look?

jerryshao · 2024-09-24T01:48:47Z

Just make the CI pass, I'm fine with the current PR.

…pache#4055) Add the list operation of the user Fix: apache#3348 I will add the document later. Add the new ut.

lw-yang reviewed Jul 3, 2024

View reviewed changes

jerqi force-pushed the ISSUE-3348 branch 4 times, most recently from a124b10 to 2f1476c Compare July 3, 2024 11:45

jerqi closed this Jul 4, 2024

jerqi reopened this Jul 4, 2024

lw-yang reviewed Jul 4, 2024

View reviewed changes

jerqi force-pushed the ISSUE-3348 branch 2 times, most recently from 9d07ea0 to a57e4e3 Compare July 16, 2024 06:56

jerryshao reviewed Jul 17, 2024

View reviewed changes

jerqi force-pushed the ISSUE-3348 branch from 88e3a3b to fba57b2 Compare July 18, 2024 07:25

jerryshao reviewed Jul 18, 2024

View reviewed changes

jerqi force-pushed the ISSUE-3348 branch 3 times, most recently from 62c5484 to 8118c63 Compare September 9, 2024 03:05

jerqi force-pushed the ISSUE-3348 branch 3 times, most recently from 4b728a5 to b2d206d Compare September 9, 2024 08:25

jerqi self-assigned this Sep 9, 2024

jerqi added the branch-0.6 label Sep 9, 2024

jerryshao reviewed Sep 14, 2024

View reviewed changes

jerqi requested a review from jerryshao September 14, 2024 02:08

Fix it

5a03882

jerqi dismissed jerryshao’s stale review via 5a03882 September 23, 2024 06:47

Fix it

6cddd60

yuqi1129 reviewed Sep 23, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/gravitino/EntityStore.java Outdated Show resolved Hide resolved

jerqi added 3 commits September 23, 2024 16:57

change log level

c2c5c22

Change type

7f5bf9b

fix CI

882d70d

xunliu reviewed Sep 23, 2024

View reviewed changes

rename

f246628

yuqi1129 reviewed Sep 23, 2024

View reviewed changes

jerqi added 6 commits September 23, 2024 20:37

Remove double deny

51f6b00

fix

50170d1

fix

162b64a

rename

a3a7865

rename

39e100d

rename

154a64b

xunliu reviewed Sep 23, 2024

View reviewed changes

jerqi added 2 commits September 23, 2024 22:54

modify comment

c74bf55

use unmodified collection

457172b

jerqi closed this Sep 24, 2024

jerqi reopened this Sep 24, 2024

yuqi1129 approved these changes Sep 24, 2024

View reviewed changes

yuqi1129 merged commit 92a0ec8 into apache:main Sep 24, 2024
54 of 55 checks passed

yuqi1129 deleted the ISSUE-3348 branch September 24, 2024 02:46

jerqi mentioned this pull request Sep 24, 2024

[#4873] feat(core): support list group #4879

Merged

jerqi added a commit to qqqttt123/gravitino that referenced this pull request Sep 24, 2024

[apache#3348] feat(core,server): Add the list operation of the user (a…

e515a05

…pache#4055) Add the list operation of the user Fix: apache#3348 I will add the document later. Add the new ut.

[#3348] feat(core,server): Add the list operation of the user #4055

[#3348] feat(core,server): Add the list operation of the user #4055

Conversation

jerqi commented Jul 3, 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

jerqi commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Jul 12, 2024

jerqi commented Jul 16, 2024

Choose a reason for hiding this comment

jerqi Jul 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Sep 9, 2024

xunliu commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Sep 14, 2024 • edited Loading

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

jerryshao commented Sep 23, 2024

yuqi1129 commented Sep 23, 2024

jerqi commented Sep 23, 2024

Choose a reason for hiding this comment

jerqi Sep 23, 2024 • edited Loading

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

jerqi Sep 23, 2024 • edited Loading

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

xunliu left a comment

Choose a reason for hiding this comment

jerryshao commented Sep 24, 2024

jerqi Jul 17, 2024 •

edited

Loading

jerqi Sep 14, 2024 •

edited

Loading

jerqi Sep 23, 2024 •

edited

Loading

jerqi Sep 23, 2024 •

edited

Loading