----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47181/#review132649 -----------------------------------------------------------
Fix it, then Ship it! Looks good. A couple of minor questions... notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java (lines 63 - 67) <https://reviews.apache.org/r/47181/#comment196923> Just for organization, could this be moved under Constructors with the public constructor? Also, if it's only been added for testing could the scope be made package or protected? notification/src/main/java/org/apache/atlas/notification/AbstractNotificationConsumer.java (lines 72 - 75) <https://reviews.apache.org/r/47181/#comment196924> Do we need an empty implementation here or could it be left as abstract? - Tom Beerbower On May 10, 2016, 1:04 p.m., Hemanth Yamijala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47181/ > ----------------------------------------------------------- > > (Updated May 10, 2016, 1:04 p.m.) > > > Review request for atlas, Shwetha GS and Tom Beerbower. > > > Bugs: ATLAS-629 > https://issues.apache.org/jira/browse/ATLAS-629 > > > Repository: atlas > > > Description > ------- > > The attached patch makes the following changes: > > * Disables auto commit in Kafka consumer properties. > * Modifies the creation of Kafka consumer objects like ConsumerConnector, > KafkaConsumer and HookConsumer to enable manual commit. > * Modifies HookConsumer to commit after a message is processed. > * Adds/modifies unit tests to accommodate changes. > > > Diffs > ----- > > distro/src/conf/atlas-application.properties 119865d > notification/src/main/java/org/apache/atlas/kafka/KafkaConsumer.java > f1c9742 > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > cfffec4 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java > 7d22126 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotificationConsumer.java > f00bbca > > notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java > 78e8ce7 > notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java > 7f607c6 > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java > 17fda25 > typesystem/src/main/resources/atlas-application.properties a343a20 > > webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java > 8ef2f64 > > webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java > 8765826 > > Diff: https://reviews.apache.org/r/47181/diff/ > > > Testing > ------- > > * Ran 2 instances of Atlas server and stopped instances with kill -9 6 times > in total, while ingesting 90 hive tables in a loop via beeline. > * Verified that all 90 tables are imported. Also verified a message > processing that stopped abruptly in between was picked up by the new instance > and completed. > * Additional UTs added. All existing tests pass. > > > Thanks, > > Hemanth Yamijala > >
