-
Notifications
You must be signed in to change notification settings - Fork 68
Optimize user list #6803
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.
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
base: main
Are you sure you want to change the base?
Optimize user list #6803
Conversation
0d215f6 to
3c529c2
Compare
0bdf2f6 to
4cc085b
Compare
4c65034 to
4dea1c8
Compare
e581209 to
39de045
Compare
39de045 to
7571ec4
Compare
solth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BartChris thank you very much, this does seem to improve the performance of the user list siginicantly. As far as I could test the caches did not create any erroneous lists. I just have a few suggestions concerning the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some imports become unused and should be removed after the proposed changes:
org.kitodo.data.database.beans.Clientorg.kitodo.data.database.beans.Projectorg.kitodo.data.database.beans.Roleorg.kitodo.production.services.data.ClientServiceorg.kitodo.production.services.data.RoleService
| */ | ||
| public void checkAndDelete() { | ||
| if (getTasksInProgress(userObject).isEmpty()) { | ||
| if (!hasTasksInProgress(userObject)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this if...else condition check can be inverted to prevent the negation with ! and make it easier to read? Something like
if (hasTasksInProgress(userObject)) {
PrimeFaces.current().ajax().update("usersTabView:confirmResetTasksDialog");
PrimeFaces.current().executeScript("PF('confirmResetTasksDialog').show();");
} else {
deleteUser(userObject);
}
|
|
||
| public String getClientNames(User user) { | ||
| List<String> clients = getLazyUserModel().getClientsCache().get(user.getId()); | ||
| return (Objects.isNull(clients) || clients.isEmpty()) ? "" : String.join(", ", clients); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return (Objects.isNull(clients) || clients.isEmpty()) ? "" : String.join(", ", clients); | |
| return (Objects.isNull(clients) || clients.isEmpty()) ? "" : String.join(", ", clients); |
| return (LazyUserModel) lazyBeanModel; | ||
| } | ||
|
|
||
| public String getRoleTitles(User user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add javadoc to public methods (applies to all new public methods below as well)
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid wildcard imports and replace with single class imports
| Map<String, Object> params = new HashMap<>(); | ||
| params.put("ids", userIds); | ||
| List<Object[]> rows = dao.getProjectionByQuery(hql, params); | ||
| Map<Integer, List<String>> result = new HashMap<>(); | ||
| for (Object[] row : rows) { | ||
| Integer userId = (Integer) row[0]; | ||
| String title = (String) row[1]; | ||
| result.computeIfAbsent(userId, k -> new ArrayList<>()).add(title); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part to gather the query parameters and retrieve the result is duplicated in the individual load..ForUsers methods and could/should be moved to a separate private method to avoid code duplication.
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public List<Object> load(int first, int pageSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add javadoc
|
@thomaslow it would be great if you could take a look at this pull request, too, since it does touch on several aspects you refactored in #6770 |
thomaslow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR and couldn't find any issues. Unfortunately, I do not have that many active users (7) in my test database, so I can at the moment only measure performance improvements in milliseconds (reduction from 150ms to 70ms).
Anyway, the approach of querying roles, clients and projects in batches is of course reasonable and should significantly improve performance. The implementation looks good. I generally agree with @solth's comments regarding some minor code style improvements (Javadocs of public methods, unused imports, etc.).
@solth I think it would be easier to merge these changes into #6770 than the other way around. So it would be good to merge this PR earlier than #6770.
Fixes #4378
This Pull Request tries to adress the slow loading user list by using optimized SQL queries to retrieve the information we need for the user list.
Some aspects of the PR have already been discussed here. #6802
One aspect is whether the SQL querie for loading tasks "in processing" is too restrictive as it only looks for tasks where the user is actually the processing user.
https://github.com/BartChris/kitodo-production/blob/96c5e8e32408acd42e414309e2f9810e45d13a59/Kitodo/src/main/java/org/kitodo/production/services/data/UserService.java#L660-L665
However, i think this was the case before with the old implementation:
kitodo-production/Kitodo/src/main/java/org/kitodo/production/services/data/UserService.java
Lines 470 to 475 in 96c5e8e
which only retrieved tasks where the user is set as
processing user, if i am not mistaken. I believe this behavior is correct, as this check is only relevant in the user list when determining whether a user has tasks in progress, for example when deciding if the user can be deleted.The other question was why we call Primefaces from the
LazyUsermodel(see: #4378 (comment)). We however also do that in theLazyBeanModeland theLazyProcessModelkitodo-production/Kitodo/src/main/java/org/kitodo/production/model/LazyProcessModel.java
Line 146 in 53eec68
kitodo-production/Kitodo/src/main/java/org/kitodo/production/model/LazyBeanModel.java
Line 131 in 53eec68
I basically adapted the implementation from there, so this maybe deserves a seperate discussion.