----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45600/#review126662 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/model/RangerTag.java (line 44) <https://reviews.apache.org/r/45600/#comment189672> Since this object is serialized (to/from Json), use Short instead of short. security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 92) <https://reviews.apache.org/r/45600/#comment189679> Looks like createOrUpdate is never set to 'false'. In that case, consider removing this variable and code its usage in rest of the method - to simplify the code. security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 124) <https://reviews.apache.org/r/45600/#comment189678> Looks like tagDefsInStore would never be null (line #89). Please review and remove this null-check if unnecessary. security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 157) <https://reviews.apache.org/r/45600/#comment189680> shouldn't the lookup based on resourceSignature be restricted to a service? To avoid incorrect updates to same named resources (like database=finance) in 2 different services (prod_hive, test_hive) security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 234) <https://reviews.apache.org/r/45600/#comment189681> shouldn't ID of the creted tag be added to associatedTagIds? Also review other blocks that create a tag. Consider using another list to track the tags that need to be saved - like tagsToRetain; populate this list as you iterate to tagIds. At the end of processing delete the tags that are in associatedTagIds but not in tagsToRetain. security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 262) <https://reviews.apache.org/r/45600/#comment189682> why remove it from associatedTags? security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 277) <https://reviews.apache.org/r/45600/#comment189675> shouldn't the existing resource-private tag be removed in this case? security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 294) <https://reviews.apache.org/r/45600/#comment189676> can this query be replaced with "associatedTagIds.contains(matchingTag.getId())"? security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java (line 361) <https://reviews.apache.org/r/45600/#comment189683> This would end up retrieving the same tag multiple times. Consider using "List<RangerTag> existingTags" - instead of tagIds. security-admin/src/main/resources/META-INF/jpa_named_queries.xml (line 894) <https://reviews.apache.org/r/45600/#comment189673> Consider replacing 0 with a parameter named :owner; and rename findPrivateTagsByServiceId ==> findTagsByOwnerAndServiceId. security-admin/src/main/resources/META-INF/jpa_named_queries.xml (line 912) <https://reviews.apache.org/r/45600/#comment189674> Consider replacing 0 with a parameter named :owner; and rename findPrivateTagAttributesByServiceId ==> findTagAttributesByOwnerAndServiceId. - Madhan Neethiraj On April 1, 2016, 7:31 p.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45600/ > ----------------------------------------------------------- > > (Updated April 1, 2016, 7:31 p.m.) > > > Review request for ranger and Madhan Neethiraj. > > > Bugs: RANGER-903 > https://issues.apache.org/jira/browse/RANGER-903 > > > Repository: ranger > > > Description > ------- > > Optimized processing of ServiceTags to minimize creation/deletion of > Tag-related objects > > > Diffs > ----- > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerTag.java > 78040ba > > agents-common/src/main/java/org/apache/ranger/plugin/store/AbstractTagStore.java > 7d2e130 > agents-common/src/main/java/org/apache/ranger/plugin/store/TagStore.java > 69bd628 > > agents-common/src/main/java/org/apache/ranger/plugin/store/file/TagFileStore.java > 58052cf > agents-common/src/main/java/org/apache/ranger/plugin/util/ServiceTags.java > 738a947 > > agents-common/src/test/java/org/apache/ranger/plugin/store/TestTagStore.java > aaace89 > security-admin/db/sqlserver/patches/021-update-tag-for-owner.sql dabe841 > > security-admin/src/main/java/org/apache/ranger/biz/RangerTagDBRetriever.java > 3b7555e > security-admin/src/main/java/org/apache/ranger/biz/TagDBStore.java cc2386e > > security-admin/src/main/java/org/apache/ranger/common/RangerServiceTagsCache.java > cdc44e0 > security-admin/src/main/java/org/apache/ranger/db/XXTagAttributeDao.java > c993477 > security-admin/src/main/java/org/apache/ranger/db/XXTagDao.java 3c9370b > security-admin/src/main/java/org/apache/ranger/entity/XXTag.java 526557e > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > 0dbd042 > > security-admin/src/main/java/org/apache/ranger/rest/ServiceTagsProcessor.java > ebe71eb > security-admin/src/main/resources/META-INF/jpa_named_queries.xml 2bb66ca > tagsync/samples/tags.json 3028f9d > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlas/AtlasNotificationMapper.java > 2168983 > tagsync/src/main/resources/etc/ranger/data/tags.json b4cd736 > > Diff: https://reviews.apache.org/r/45600/diff/ > > > Testing > ------- > > Uploaded ServiceTags containing Tag objects which already existed in the > Ranger. Ensured that existing objects where updated wherever necessary, but > no unnecessary deletions/creations were done. > > > Thanks, > > Abhay Kulkarni > >
