> On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > 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.
Thanks Madhan - I have fixed all but 3 of the issues, which I'm leaving open for now. I raised ATLAS-2844 for the eventTime one, need some extra help from you on the immutable superclass one, and I need to work out what to do about the third one around cardinality. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > > Lines 179 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074622#file2074622line179> > > > > 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. Thanks for the suggestion - that was actually how I originally implemented it - but there is a fundamental problem, which is that the entity change notification listener sends the Kafka publish prior to committing the transaction. The event mapper doesn't know whether it is seeing current or stale state, it would have to assume it is current. The alternative implementation (as I have it now) is to sleep briefly prior to each batch of messages (rather than individual messages) in order to yield and give the entitiy change notifier thread a chance to commit the entity update. This is not guaranteed to succeed of course, but the loss of the event would not be a show-stopper. The better solution would be that the entity change notification listener should really commit the update prior to the delivery of the notification. What happens currently is the publish to Kafka and the resulting consumption from Kafka and triggering of the event mapper all occur prior to the commit (or rollback). > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > > Lines 181 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074622#file2074622line181> > > > > It is generally not a good idea to catch & ignore > > 'InterruptedException'. This can potentially prevent the JVM from shutting > > down. Thanks - I have changed this to insert an error entry in the log and set keepOnRunning to false - hence terminating the loop. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > > Lines 359 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074622#file2074622line359> > > > > line #351 and #359 will result in retrieving the entity twice from the > > store; consider avoiding this duplicate retrieval - for better performance. Yes - thanks - I am aware of the double retrieve. I have decided to keep it that way for now at least. We could change it if there is a performance problem, but the current approach avoids adding a public method to the metdata collection API that would retrieve the AtlasEntity. The event mapper and repository connector are in separate packages so I can't make the methods package-private. The other option would be to use a compound (public) method that asks both questions in one method call, e.g. something like getEntityCharacteristics - which would allow us to test whether it is local (or not) and a proxy (or not). I prefer the pair of simpler methods. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > > Lines 367 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074622#file2074622line367> > > > > 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. I have enhanced the event mapper to find out whether Atlas performing hard or soft deletes, and behave appropriately. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java > > Lines 389 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074622#file2074622line389> > > > > 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. OK - can do. I have raised ATLAS-2844 and will leave this review comment open. The solution to that JIRA will have to decide when the change actually happens - it could be at commencement of entity store processing, or it could be at commit time, or at some point between the two. As described above the event mapper will currently wait before processing each batch of messages, so there will always be a delay. Once ATLAS-2844 is resolved we can improve precision. This code has also changed as a result of one of your other review comments, and the event mapper now does not fill in the timestamp, so we would also need a means to pass the actual event time (from Atlas) to Egeria. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java > > Lines 62 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074623#file2074623line62> > > > > 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; > > } Can you please elaborate on why this is better? I am famniliar with using private constructors for singletons, but I don't see why, in this case, the use of a private superclass constructor helps - in either case it seems that the only way to initialise the class name and type are through the (public) provider constructor. Thanks! :-) > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java > > Lines 60 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074624#file2074624line60> > > > > 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. Thanks for that - I have modified the whole module to use the registry when it is retrieving a type; but continued to use the type def store for updates and searches. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java > > Lines 125 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074624#file2074624line125> > > > > convertAtlasAttributeDef() can return null. Should such entries be > > added to the list returned from here? I think that is only in the case the the AtlasAttributeDef is null - but I've wrapped some protection around the add to the list. Thanks for spotting it. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java > > Lines 129 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074624#file2074624line129> > > > > 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. Thanks - I mostly had included at least the exception message (in other classes) but have updated whole module to include exception. > On Aug. 26, 2018, 5:28 p.m., Madhan Neethiraj wrote: > > open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java > > Lines 190 (patched) > > <https://reviews.apache.org/r/68412/diff/1/?file=2074624#file2074624line190> > > > > 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 OK - I need to look into this and talk to the Egeria community about it. Will leave this issue open and address it asap. - Graham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68412/#review207941 ----------------------------------------------------------- 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 > >
