----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45720/#review127308 -----------------------------------------------------------
distro/src/conf/atlas-log4j.xml (line 48) <https://reviews.apache.org/r/45720/#comment190569> Its probably OK to write to a log file for now, but going forward, we do need to write to a shared store - so that it would work in multiple server scenario, right? Can we file a JIRA for this so that we can keep track? notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java (line 1) <https://reviews.apache.org/r/45720/#comment190591> Missing Apache Licence header. notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java (line 34) <https://reviews.apache.org/r/45720/#comment190589> Can you please validate my understanding? There were a bunch of deserializers that were present in AbstractNotification. As part of this patch, I think you have moved these such that specific deserializers are added only as required, rather than all being registered at one place. Is that right? notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java (line 56) <https://reviews.apache.org/r/45720/#comment190592> Did we not need these serializers before? notification/src/main/java/org/apache/atlas/notification/IncompatableVersionException.java (line 25) <https://reviews.apache.org/r/45720/#comment190590> Typo: Incompat'i'bleVersionException? notification/src/main/java/org/apache/atlas/notification/MessageVersion.java (line 84) <https://reviews.apache.org/r/45720/#comment190586> May not be a case that actually happens, but would we treat 1.0 and 1 as the same version? Should we handle that? notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java (line 34) <https://reviews.apache.org/r/45720/#comment190570> I see we have two different types, one for Hook messages and one for Entity messages.. But isn't a versioned message a container that can contain a message of Type T and thus be of only one type? notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java (line 95) <https://reviews.apache.org/r/45720/#comment190580> This exception seems to be stopping the message consuming thread. That would be a problem. notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java (line 100) <https://reviews.apache.org/r/45720/#comment190581> In a backwards compatible environment, the volume of messages of older version may be high, so should we warn at lower level (INFO maybe)? notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java (line 37) <https://reviews.apache.org/r/45720/#comment190593> Is there a specific value this test is adding over KafkaConsumerTest? - Hemanth Yamijala On April 5, 2016, 1:07 a.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45720/ > ----------------------------------------------------------- > > (Updated April 5, 2016, 1:07 a.m.) > > > Review request for atlas. > > > Bugs: ATLAS-631 > https://issues.apache.org/jira/browse/ATLAS-631 > > > Repository: atlas > > > Description > ------- > > 1. Introduce Versioning to Atlas Notification Payload (both ways) > 2. For any messages that are not able to be processed, log the message do a > separate log file for unprocessed messages. > > > Diffs > ----- > > distro/src/conf/atlas-log4j.xml 1ac4082 > notification/src/main/java/org/apache/atlas/kafka/KafkaConsumer.java > 029a072 > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 889af11 > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java > 596f988 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotificationConsumer.java > 1cadb99 > > notification/src/main/java/org/apache/atlas/notification/IncompatableVersionException.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/MessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java > ac285aa > > notification/src/main/java/org/apache/atlas/notification/VersionedMessage.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/entity/EntityMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/hook/HookMessageDeserializer.java > PRE-CREATION > notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java > db34815 > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/MessageVersionTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/VersionedMessageTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityMessageDeserializerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationImplTest.java > 385c41f > > notification/src/test/java/org/apache/atlas/notification/hook/HookMessageDeserializerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/hook/HookNotificationTest.java > 11b7a53 > typesystem/src/main/resources/atlas-log4j.xml 2bb49d3 > > Diff: https://reviews.apache.org/r/45720/diff/ > > > Testing > ------- > > New unit tests added. > > mvn clean test. > > Additional testing in progress. > > > Thanks, > > Tom Beerbower > >
