> On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > client/src/main/java/org/apache/atlas/PropertiesUtil.java, line 42 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016152#file1016152line42> > > > > Merge may have conflicts, no? Isnt this dangerous? Why cant we treat > > this separate configs?
kafka related configs like endpoint etc need to be shared between client and server. Don't want to define twice(both in client.properties and application.properties) and handle discrepancy issues later. Want to get rid of client.properties once we have consensus. Re-using client configs till then this way > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > pom.xml, line 976 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016162#file1016162line976> > > > > Why is this necessary? For embedded kafka > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > notification/src/main/java/org/apache/atlas/notification/ConsumerInterface.java, > > line 20 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016156#file1016156line20> > > > > Seriously, suffix with Interface? :-) > > > > How close is this to JMS? Didn't have a better name, will rename JMS has receive(timeout). But kafka doesn't expose this interface currently. The one added here is the only kafka consumer interface. The new receive(timeout) interface is still in the works, we can change once thats released > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > notification/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java, > > line 66 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016158#file1016158line66> > > > > There is an implicit assumption that we always create the entity json > > and put it in the broker? > > > > Should we verify the layload? Still need work in this. Just added basic kafka messaging as part of this patch. Will finalise the hook message format in another patch > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java, > > line 26 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016159#file1016159line26> > > > > Seriously, suffix with Interface? :-) I don't have any better name, please suggest:) > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java, > > line 33 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016159#file1016159line33> > > > > Should we also allow Traits as a specific notofication type? > > > > Hook generalizes all components - do we need anything specific here? > > Cant think of any ATM Didn't want to expose topic name to clients and hence added notification type. Yes, we need traits. Will add as part of entity/type notification. Don't want to add any component specific part in the hook message. Else, we need to depend on all components and their internals? > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > notification/src/main/java/org/apache/atlas/notification/NotificationModule.java, > > line 26 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016160#file1016160line26> > > > > Is there a way to externalize this? How do folks say wrap another > > implementaiton - for future. I didn't want to add to just config as we use Guice everywhere. Add to config and use that in Guice? Makes sense, will change. > On July 20, 2015, 11:16 p.m., Seetharam Venkatesh wrote: > > src/conf/application.properties, line 32 > > <https://reviews.apache.org/r/36611/diff/1/?file=1016163#file1016163line32> > > > > Dont we need the notification impl type here which can be used in Guice? Will add - Shwetha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36611/#review92333 ----------------------------------------------------------- On July 20, 2015, 6:14 a.m., Shwetha GS wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36611/ > ----------------------------------------------------------- > > (Updated July 20, 2015, 6:14 a.m.) > > > Review request for atlas. > > > Bugs: ATLAS-74 > https://issues.apache.org/jira/browse/ATLAS-74 > > > Repository: atlas > > > Description > ------- > > Add notification framework with kafka as default implementation > > > Diffs > ----- > > client/src/main/java/org/apache/atlas/PropertiesUtil.java bc56cbf > notification/pom.xml PRE-CREATION > notification/src/main/java/org/apache/atlas/kafka/KafkaConsumer.java > PRE-CREATION > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/ConsumerInterface.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationException.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationModule.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/kafka/kafkaNotificationTest.java > PRE-CREATION > pom.xml 6e7c10c > src/conf/application.properties 6c4c7d2 > typesystem/src/main/resources/application.properties 29c933f > typesystem/src/main/resources/log4j.xml 742f674 > webapp/src/main/java/org/apache/atlas/Main.java 1dd17b4 > webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java > 88200f0 > webapp/src/main/resources/application.properties c9b8408 > > Diff: https://reviews.apache.org/r/36611/diff/ > > > Testing > ------- > > UTs > > > Thanks, > > Shwetha GS > >
