----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65700/#review197719 -----------------------------------------------------------
dashboardv2/public/js/utils/Enums.js Lines 31 (patched) <https://reviews.apache.org/r/65700/#comment277934> How are these different from existing enums TAG_ADD/TAG_DELETE/TAG_UPDATE? graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasVertex.java Lines 59 (patched) <https://reviews.apache.org/r/65700/#comment277935> "If the property is already present, it is added again and no exception is thrown." - perhaps it should be: "if the property is present, the given value will be added to its existing value. If property is not present, it will be created with the given value" intg/src/main/java/org/apache/atlas/audit/EntityAuditEventV2.java Lines 44 (patched) <https://reviews.apache.org/r/65700/#comment277936> Consider moving this class to "models" package, as this is referenced in REST APIs. intg/src/main/java/org/apache/atlas/audit/EntityAuditEventV2.java Lines 55 (patched) <https://reviews.apache.org/r/65700/#comment277938> What is the "details" field used for? If this is not necessary, consider removing it. intg/src/main/java/org/apache/atlas/audit/EntityAuditEventV2.java Lines 57 (patched) <https://reviews.apache.org/r/65700/#comment277937> "entityDefinition" ==> "entity" intg/src/main/java/org/apache/atlas/model/instance/AtlasClassification.java Lines 55 (patched) <https://reviews.apache.org/r/65700/#comment277939> Is it necessary to assign an internal-guid for classifications? Consider setting this to null by default. intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java Lines 271 (patched) <https://reviews.apache.org/r/65700/#comment277940> Consider inserting the following line after #270 and removing #273: this.classificaitons = c; intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java Lines 282 (patched) <https://reviews.apache.org/r/65700/#comment277941> Similar to addClassifications(), consider adding method addPropagatedClassifications(). intg/src/main/java/org/apache/atlas/v1/model/notification/EntityNotificationV2.java Lines 57 (patched) <https://reviews.apache.org/r/65700/#comment277942> 'entity' already has fields classifications & propagatedClassifications. does 'allClassifications' include both these lists, and also have super-type classification instances? repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 439 (patched) <https://reviews.apache.org/r/65700/#comment277945> Multiple edges for the given classification name can exists - consider multiple classifications of the same name propagated from different entities. To handle such cases, all the edges returned should be iterated. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 457 (patched) <https://reviews.apache.org/r/65700/#comment277946> If vertex doesn't include CLASSIFICATION_PROPAGATE_KEY property, this line might hit NPE. Consider assigning the return from AtlasGraphUtilsV1.getProperty() to a Boolean value; and treat null as "true". repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1125 (patched) <https://reviews.apache.org/r/65700/#comment277947> Is this call to getAllTraitNames() necessary here? Query in this call is duplocated in the call to getAllClassificationVertices() below - line #1128. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1135 (patched) <https://reviews.apache.org/r/65700/#comment277952> LOG.info ==> LOG.debug, within LOG.isDebugEnabled(). Please update other such occurances as well. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1138 (patched) <https://reviews.apache.org/r/65700/#comment277959> For efficiency, consider moving lines #1138-#1140 after the 'if' block at #1142 i.e. if the classification is not to be propagated, no need to retrieve these details. Same is application for removeTagPropagation() method below as well. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1147 (patched) <https://reviews.apache.org/r/65700/#comment277960> Consider moving this 'if' bock just before this 'for' loop i.e. when impactedEntityVertices is empty, there is no need to enter this 'for' loop. Same is application for removeTagPropagation() method below as well. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1155 (patched) <https://reviews.apache.org/r/65700/#comment277961> Given there will be only one edge between a classification vertex and an entity vertex, it might be efficient to get the edgeLabel and compare with classificationName/propagatedEdgeLabel - instead of performing 2 searches in line #1155 & #1161. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1180 (patched) <https://reviews.apache.org/r/65700/#comment277948> Is this call to getAllTraitNames() necessary here? Query in this call is duplocated in the call to getAllClassificationVertices() below - line #1183. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java Lines 1232 (patched) <https://reviews.apache.org/r/65700/#comment277962> Multiple classification instances of a given name can be in propagated to an entity. Please review if removePropagatedTraitNameFromVertex() handles this case correctly. webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java Lines 393 (patched) <https://reviews.apache.org/r/65700/#comment277944> Having propagatedClassifications included in existing AtlasEntity.classifications will eliminate the need for methods such as getPropagatedClassifications(); and also wouldn't require review/update to existing usage of AtlasEntity.classifications - to handle propagatedClassfications list as well. webapp/src/test/resources/atlas-application.properties Lines 67 (patched) <https://reviews.apache.org/r/65700/#comment277943> Consider renaming this property as below: atlas.notification.entity.version=v1 - Madhan Neethiraj On Feb. 17, 2018, 5:07 a.m., Sarath Subramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65700/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2018, 5:07 a.m.) > > > Review request for atlas, Apoorv Naik, Ashutosh Mestry, and Madhan Neethiraj. > > > Bugs: ATLAS-2456 > https://issues.apache.org/jira/browse/ATLAS-2456 > > > Repository: atlas > > > Description > ------- > > Scalable way to quickly and efficiently propagate tags for efficient searches > and tag based security. Likewise tags for derivative dataset should be > inherited from the parent. For example, if an entity is tagged "PII" then > resulting entity created from a CTAS operation should also be tagged "secret" > to maintain the classification of the parent. In the case where 2 or more > datasets are aggregated the derivative dataset should be a union of all > parent tags. > > This changeset includes introduction of v2 data structures for notifications > and audit events. > > > Diffs > ----- > > addons/models/0000-Area0/0010-base_model.json 0296e8f9 > common/src/main/java/org/apache/atlas/repository/Constants.java 265be788 > dashboardv2/public/js/utils/Enums.js a7b9a8b6 > > graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasElement.java > 42837f49 > > graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasVertex.java > a68d8ebe > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusVertex.java > aef20f03 > > graphdb/titan0/src/main/java/org/apache/atlas/repository/graphdb/titan0/Titan0Vertex.java > ca48e3d9 > intg/src/main/java/org/apache/atlas/AtlasErrorCode.java ff09e6c9 > intg/src/main/java/org/apache/atlas/audit/EntityAuditEventV2.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/listener/EntityChangeListenerV2.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/model/instance/AtlasClassification.java > f594a814 > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java > 08d1ce11 > > intg/src/main/java/org/apache/atlas/model/notification/EntityNotification.java > b272b733 > > intg/src/main/java/org/apache/atlas/v1/model/notification/EntityNotificationV2.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListener.java > 74d3b913 > > repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListenerV2.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditRepository.java > 9dc78350 > > repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java > 774934c7 > > repository/src/main/java/org/apache/atlas/repository/audit/InMemoryEntityAuditRepository.java > 22d2a810 > > repository/src/main/java/org/apache/atlas/repository/audit/NoopEntityAuditRepository.java > c3826019 > > repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java > 2884f8f0 > > repository/src/main/java/org/apache/atlas/repository/graph/FullTextMapperV2.java > c42aa156 > > repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java > 31620b1e > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > d61bff24 > > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java > 79e8e3e8 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityChangeNotifier.java > 2b6beade > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > ca0eeeb6 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java > 7389f49b > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > 779bc387 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java > b05a9a32 > > repository/src/main/java/org/apache/atlas/util/AtlasGremlin3QueryProvider.java > 1fda2415 > > repository/src/main/java/org/apache/atlas/util/AtlasGremlinQueryProvider.java > a79abaaf > > repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java > 0e1e5b6c > repository/src/test/java/org/apache/atlas/TestModules.java 13bdcb08 > > repository/src/test/java/org/apache/atlas/repository/audit/AuditRepositoryTestBase.java > 47a61ee2 > > webapp/src/main/java/org/apache/atlas/notification/EntityNotificationListenerV2.java > PRE-CREATION > > webapp/src/main/java/org/apache/atlas/notification/NotificationEntityChangeListener.java > 396a2920 > webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java > 715a54db > webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java 21402e19 > webapp/src/test/java/org/apache/atlas/web/adapters/TestEntityREST.java > ea6fe313 > webapp/src/test/resources/atlas-application.properties d9009166 > > > Diff: https://reviews.apache.org/r/65700/diff/1/ > > > Testing > ------- > > mvn clean install -> succeeded in 17:11 min > > UTs/ITs for tag propagation work in progress > > > Thanks, > > Sarath Subramanian > >
