> On Dec. 4, 2024, 4:08 a.m., Abhishek Kumar wrote: > > security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java > > Lines 287 (patched) > > <https://reviews.apache.org/r/75294/diff/1/?file=2295898#file2295898line287> > > > > Suggested to use CollectionUtils.isNotEmpty()
Agreed. Will change > On Dec. 4, 2024, 4:08 a.m., Abhishek Kumar wrote: > > security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java > > Lines 334 (patched) > > <https://reviews.apache.org/r/75294/diff/1/?file=2295898#file2295898line334> > > > > Lambas with stream can be used here as well: > > > > List<Role> ret = xxRoles.stream() > > .map(xxRole -> roleService.read(xxRole.getId())) > > .collect(Collectors.toList()); Streams aren't always more performant than traditional loops. Moreover, there is no golden rule that streams should be used always. Sometimes, traditional loop is more performant from what I understand. Also, above one is just one intermediate operation and so I don't see too much benefit in changing it, dropping the issue. Would definitely have agreed if there are more intermediate operations. > On Dec. 4, 2024, 4:08 a.m., Abhishek Kumar wrote: > > security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java > > Lines 340 (patched) > > <https://reviews.apache.org/r/75294/diff/1/?file=2295898#file2295898line340> > > > > Lambdas with stream can be used here as well: > > > > Set<RangerRole> deletedRoles = cachedRangerRoles.stream() > > .filter(rangerRole -> > > !currentRoleIds.contains(rangerRole.getId())) > > .collect(Collectors.toSet()); Streams aren't always more performant than traditional loops. Moreover, there is no golden rule that streams should be used always. Sometimes, traditional loop is more performant from what I understand. Also, above one is just one intermediate operation and so I don't see too much benefit in changing it, dropping the issue. Would definitely have agreed if there are more intermediate operations. > On Dec. 4, 2024, 4:08 a.m., Abhishek Kumar wrote: > > security-admin/src/main/java/org/apache/ranger/db/XXRoleDao.java > > Lines 148 (patched) > > <https://reviews.apache.org/r/75294/diff/1/?file=2295900#file2295900line148> > > > > Suggested to use criteriaBuilder as the variable name. Agreed. Will change. - Guru Thejus ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75294/#review227106 ----------------------------------------------------------- On Dec. 2, 2024, 7:41 a.m., Guru Thejus Arveti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75294/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2024, 7:41 a.m.) > > > Review request for ranger, Pradeep Agrawal and Ramesh Mani. > > > Repository: ranger > > > Description > ------- > > RANGER-5013: Added Support for Ranger Roles Delta Sync > > > Diffs > ----- > > agents-common/src/main/java/org/apache/ranger/plugin/store/RoleStore.java > 22e1e6e65 > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRolesProvider.java > 0caf65826 > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdmin.java > 15a1e7118 > > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdminCache.java > 5bd3a0934 > > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdminImpl.java > 2434db171 > security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java > 930147b06 > security-admin/src/main/java/org/apache/ranger/common/RangerRoleCache.java > 933104a16 > security-admin/src/main/java/org/apache/ranger/db/XXRoleDao.java 4e5b692b5 > security-admin/src/main/java/org/apache/ranger/rest/RoleREST.java d8e30b516 > > security-admin/src/main/java/org/apache/ranger/service/RangerRoleServiceBase.java > 22867cbe5 > security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml > 1af339010 > > > Diff: https://reviews.apache.org/r/75294/diff/1/ > > > Testing > ------- > > Testing manually in local but adding/removing/updating ranger roles from > ranger admin UI and checking whether the roles download api is returning only > the delta and subsequently made changes in the client code > > > Thanks, > > Guru Thejus Arveti > >
