----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68412/#review207941 -----------------------------------------------------------
Graham - here are my comments from partial review; if you would like to discuss any in further detail, please let me knwo. I will continue reviewing. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java Lines 179 (patched) <https://reviews.apache.org/r/68412/#comment291592> Instead of waiting for 1 second before processing every message, it might be useful to add a retry inside processMessage() - can result in better message processing throughput. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java Lines 181 (patched) <https://reviews.apache.org/r/68412/#comment291591> It is generally not a good idea to catch & ignore 'InterruptedException'. This can potentially prevent the JVM from shutting down. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java Lines 359 (patched) <https://reviews.apache.org/r/68412/#comment291593> line #351 and #359 will result in retrieving the entity twice from the store; consider avoiding this duplicate retrieval - for better performance. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java Lines 367 (patched) <https://reviews.apache.org/r/68412/#comment291594> While processing ENTITY_DELETE notificaiton, getByGuid() call on Atlas would fail if Atlas is configured for hard-delete. Connector should be able to process this notification only with entity-guid. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java Lines 389 (patched) <https://reviews.apache.org/r/68412/#comment291595> eventTime should come from Atlas in 'notification' - given OMRS connector might process the message at a much later time (than when the notification was generated). Consider filing an Atlas JIRA to add timestamp in EntityNotification. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java Lines 39 (patched) <https://reviews.apache.org/r/68412/#comment291596> For 'static final' members, consider using upper class names. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java Lines 62 (patched) <https://reviews.apache.org/r/68412/#comment291597> It seems super.connectorClassName and super.connectorTypeBean might be set only once i.e. not mutable. If yes, consider setting this via a protected constructor: public AtlasOMRSRepositoryEventMapperProvider() { super(AtlasOMRSRepositoryEventMapper.class.getName(), initConnectorType()); } private ConnectorType initConnectorType() { ConnectorType connectorType = new ConnectorType(); connectorType.setType(ConnectorType.getConnectorTypeType()); connectorType.setGUID(connectorTypeGUID); connectorType.setQualifiedName(connectorTypeName); connectorType.setDisplayName(connectorTypeName); connectorType.setDescription(connectorTypeDescription); connectorType.setConnectorProviderClassName(this.getClass().getName()); return connectorType; } open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 57 (patched) <https://reviews.apache.org/r/68412/#comment291598> It seems these members are immutable; if yes, please consider marking them as 'final'. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 60 (patched) <https://reviews.apache.org/r/68412/#comment291603> Since this module runs within Atlas, it might be easier and efficient to work with AtlasTypeRegistry - which has all types in-memory and in a more consumable way (for example, detecting whether a type is primitive or not is as simple as: attributeType.getTypeCategory() == TypeCategory.PRIMITIVE; AtlasTypeDefStore would read from store, which can/should be avoided. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 104 (patched) <https://reviews.apache.org/r/68412/#comment291599> For return-type and parameter-types, consider using possible super-most type in the hieratchy; in this case replace ArrayList with List. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 125 (patched) <https://reviews.apache.org/r/68412/#comment291602> convertAtlasAttributeDef() can return null. Should such entries be added to the list returned from here? open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 129 (patched) <https://reviews.apache.org/r/68412/#comment291600> To help in troubleshooting, consider including the exception in the error log generated - simply by adding the exception argument at the end of arguments list. LOG.error("error message {}", arg1, e); Please review rest of error logs for above. open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java Lines 190 (patched) <https://reviews.apache.org/r/68412/#comment291601> This is comment for 'cardinality' definition in openmetada: "UNORDERED" doesn't capture the uniqueness of the elements in a SET. - SET: unordered, unique elements - LIST: ordered, can contain duplicate elements - Madhan Neethiraj On Aug. 17, 2018, 2:38 p.m., Graham Wallis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68412/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2018, 2:38 p.m.) > > > Review request for atlas. > > > Repository: atlas > > > Description > ------- > > ATLAS-1773: Atlas OMRS Repository Connector > > > Diffs > ----- > > open-metadata/pom.xml PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasBaseTypeDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasClassificationDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasEntityDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasEntityMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasRelationshipDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasRelationshipMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasStoresProxy.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasStoresProxyImpl.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/Comparator.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/DSLQueryHelper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/EntityDefMapper.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/FamousFive.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/ISpringBridge.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSErrorCode.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSMetadataCollection.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSRepositoryConnector.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSRepositoryConnectorProvider.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/SpringBridge.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TypeDefsByCategory.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TypeNameUtils.java > PRE-CREATION > > open-metadata/src/main/java/org/apache/atlas/openmetadata/admin/server/spring/OpenMetadataAdminResource.java > PRE-CREATION > > open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasClassificationDefMapper.java > PRE-CREATION > > open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasEntityDefMapper.java > PRE-CREATION > > open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasEntityMapper.java > PRE-CREATION > > open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasRelationshipDefMapper.java > PRE-CREATION > > open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasRelationshipMapper.java > PRE-CREATION > pom.xml c60a53b0e2e7038b6228e16ceba47a0080d58622 > webapp/pom.xml 78aa81e8046b676f47d24eff0671225a3c960929 > webapp/src/main/webapp/WEB-INF/openMetadataContext.xml PRE-CREATION > webapp/src/main/webapp/WEB-INF/web.xml > 23dc0637a8b521ab89a16ec8b03895cdaf8bc7d8 > > > Diff: https://reviews.apache.org/r/68412/diff/1/ > > > Testing > ------- > > Unit Testing (approx 80 tests) and Functional Testing of major operations > (creation of types, instances, reference copies and proxies). > > > Thanks, > > Graham Wallis > >
