----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59799/#review176920 -----------------------------------------------------------
Overall functionality wise it looks good. notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java Lines 45 (patched) <https://reviews.apache.org/r/59799/#comment250474> Change from fully qualified name to just class name. newKafkaConsumer -> kafkaConsumer / consumer notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java Lines 59 (patched) <https://reviews.apache.org/r/59799/#comment250475> Put this under a debug statement. notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java Lines 63 (patched) <https://reviews.apache.org/r/59799/#comment250476> Should this be logged to the error log in order to keep the main application log clean (less stacktraces) notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Line 59 (original), 68 (patched) <https://reviews.apache.org/r/59799/#comment250477> Specific imports over wildcard. notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Line 129 (original), 133 (patched) <https://reviews.apache.org/r/59799/#comment250478> Remove commented code notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Line 259 (original), 241 (patched) <https://reviews.apache.org/r/59799/#comment250479> Direct class reference instead of fully qualified one. notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Line 293 (original), 262 (patched) <https://reviews.apache.org/r/59799/#comment250480> StringUtils.isNotEmpty(groupId) notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java Lines 40 (patched) <https://reviews.apache.org/r/59799/#comment250481> Can you add Javadoc for this ? - Apoorv Naik On June 5, 2017, 8:30 a.m., Nixon Rodrigues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59799/ > ----------------------------------------------------------- > > (Updated June 5, 2017, 8:30 a.m.) > > > Review request for atlas, Apoorv Naik, Ashutosh Mestry, Madhan Neethiraj, and > Sarath Subramanian. > > > Repository: atlas > > > Description > ------- > > This patch include changes that implements kafka new-consumer api for > kafkaConsumer for Atlas Notification System. > > > Diffs > ----- > > distro/src/conf/atlas-application.properties 5e59528 > notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java > PRE-CREATION > notification/src/main/java/org/apache/atlas/kafka/KafkaConsumer.java > 16c0eb2 > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 8bd31fd > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java > 9585827 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotificationConsumer.java > d4d78de > > notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java > a99cb10 > > webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java > 2f8245d > > > Diff: https://reviews.apache.org/r/59799/diff/1/ > > > Testing > ------- > > Tested Kafka consumer on local embeded kafka /zookeeper env. > Tested Atlas Hive hook notifications. > Excecute Quick Start script. > > Due to change in kafkaconsumer api the current notification related UT/IT's > need to changed, i have not included this change in this patch but will > included it shortly. > > > Thanks, > > Nixon Rodrigues > >
