> On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > distro/src/conf/atlas-log4j.xml, line 48 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325385#file1325385line48> > > > > 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?
ok sure... ATLAS-644 > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java, > > line 34 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325388#file1325388line34> > > > > 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? Right. Basically I introduced the different MessageDeserializer classes to separate the deserialization for the different notification types (hook and entity). Not only is it cleaner to not lump all of the deserializers into one place but it allows us to handle the deserialization of the different types independently if required (different loggers, customization to support backwards compatability, etc...) > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java, > > line 56 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325389#file1325389line56> > > > > Did we not need these serializers before? We had them before. They were lumped in with the deserializers. > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/IncompatableVersionException.java, > > line 25 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325391#file1325391line25> > > > > Typo: Incompat'i'bleVersionException? Whoops. Thanks! > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java, > > line 84 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325393#file1325393line84> > > > > May not be a case that actually happens, but would we treat 1.0 and 1 > > as the same version? Should we handle that? It should be the case with the patch that 1.0 equals 1 (See the unit test in MessageVersionTest). Are you saying that they should be different versions? > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java, > > line 95 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325396#file1325396line95> > > > > This exception seems to be stopping the message consuming thread. That > > would be a problem. I think that in the HookConsumer runnable (in NotificationHookConsumer) that it catches Throwable and just logs a warning. Are you saying that throwing a runtime exception here is a bad idea? If so, what would you say is the best way to handle a version mismatch error during a consumer.next() call? I see three posibilities ... 1) throw a runtime exception. Means that the calling code needs to handle the exception. 2) throw a checked exception. Means that we need to change the signature of our consumer interface and that any existing users need to change their code to handle the exception. 3) return null. Means that existing users must change their code to handle possible null return or suffer NPE. > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java, > > line 34 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325396#file1325396line34> > > > > 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? When we deserialize the versioned message, the deserializer needs to know what T is. Because of erasure the only way to let it know is to pass in the Type... Type HOOK_VERSIONED_MESSAGE_TYPE = new TypeToken<VersionedMessage<HookNotification.HookNotificationMessage>>(){}.getType(); Type ENTITY_VERSIONED_MESSAGE_TYPE = new TypeToken<VersionedMessage<EntityNotification>>(){}.getType(); > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java, > > line 37 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325401#file1325401line37> > > > > Is there a specific value this test is adding over KafkaConsumerTest? Maybe not much. Mostly just did it for completeness. > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java, > > line 100 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325396#file1325396line100> > > > > In a backwards compatible environment, the volume of messages of older > > version may be high, so should we warn at lower level (INFO maybe)? ok. > On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote: > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java, > > line 1 > > <https://reviews.apache.org/r/45720/diff/2/?file=1325388#file1325388line1> > > > > Missing Apache Licence header. Good catch. Thanks. Does the Atlas build not do a RAT check? - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45720/#review127308 ----------------------------------------------------------- On April 6, 2016, 6:17 p.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45720/ > ----------------------------------------------------------- > > (Updated April 6, 2016, 6:17 p.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/IncompatibleVersionException.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 > >
