> On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumer.java, > > line 58 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095117#file1095117line58> > > > > I guess the reason for EntityNotificationConsumer is for next() to > > provide EntityNotification instead of String message, right? > > > > If this is the case, instead of defining another consumer for entity > > notification we need to templatise NotificationConsumer itself so that > > depending on type, consumer returns correct notification class. For > > example, for hook type, consumer.next() should return HookNotification, for > > entity notification type, consumer.next() should return EntityNotification > > and so on. That way, we don't have to define another consumer for each > > notification type. Makes sense?
Yes makes sense but I don't think that there is a HookNotification currently. Does that need to be defined still? I'm a bit worried about the size of this patch. Can I do a generic templated NotificationConsumer and have hook notifications take it up as a part of another patch? > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/pom.xml, line 87 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095114#file1095114line87> > > > > duplicate dependency Not sure how that happened. I should have caught it. Thanks. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java, > > line 334 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095115#file1095115line334> > > > > Instead of adding extra condition, always read the consumer id from > > application.properties - atlas.kafka.<topic name>.group.id with the default > > value for hook topic in application.properties. > > > > You can even add all properties from application.properties that start > > with atlas.kafka.<topic name> to the topic's consumer config Okay. That makes sense. What if there is no group id property specified for the requested topic type? Is that an error condition or do we just use some default? > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotification.java, > > line 62 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095116#file1095116line62> > > > > This should be part of the entity, not EntityNotification? What is > > values? The intent was that additional values could be supplied here that part of the notification. For example, if the notification is a TRAIT_ADDED notification then the values map would contain an entry with the name of the added trait keyed by "traitName". I used a generic map to avoid having to have different notification classes for each notification type, but yeah the usage should be documented at least. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProvider.java, > > line 58 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095118#file1095118line58> > > > > Hard coded '1' here is the client's parallelism, we should expose this > > to clients. > > > > I still think NotificationConsumer is the right interface to give it to > > clients(even ranger) which gives right controls to users and also provide > > simple enough abstraction Yes, makes sense. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationImpl.java, > > line 75 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095119#file1095119line75> > > > > The argument should be Entity? Yes, it can be Entity. I'll change that. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > repository/src/test/java/org/apache/atlas/services/DefaultMetadataServiceTest.java, > > line 48 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095133#file1095133line48> > > > > There is another org.apache.atlas.service.DefaultMetadataServiceTest. > > Lets merge to one I was confused by this. I think that the tests should be in the same package at the class being tested. The existing test is in the org.apache.atlas.service package. Why are there org.apache.atlas.service AND org.apache.atlas.services packages? I'll open a Jira to get rid of the org.apache.atlas.service package and merge the tests. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > repository/src/test/java/org/apache/atlas/services/DefaultMetadataServiceTest.java, > > line 86 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095133#file1095133line86> > > > > Don't add the json as string. Instead construct the object and use > > serialisation. With string, its difficult to change json for > > adding/updating fields ok. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > repository/src/main/java/org/apache/atlas/services/NotificationEntityChangeListener.java, > > line 94 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095128#file1095128line94> > > > > What is values here? What are the keys for the map? Same as above for EntityNotification. > On Oct. 12, 2015, 5:36 a.m., Shwetha GS wrote: > > repository/src/main/java/org/apache/atlas/services/NotificationEntityChangeListener.java, > > line 67 > > <https://reviews.apache.org/r/38341/diff/4/?file=1095128#file1095128line67> > > > > notification interface supports batch API which uses async interface. > > It should be used here Are you saying that this method should allow for multiple entity notifications and batch them together in one call to NotificationInterface.send()? - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38341/#review102205 ----------------------------------------------------------- On Oct. 12, 2015, 4:25 a.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38341/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2015, 4:25 a.m.) > > > Review request for atlas, John Speidel and Shwetha GS. > > > Bugs: ATLAS-158, ATLAS-215 and ATLAS-217 > https://issues.apache.org/jira/browse/ATLAS-158 > https://issues.apache.org/jira/browse/ATLAS-215 > https://issues.apache.org/jira/browse/ATLAS-217 > > > Repository: atlas > > > Description > ------- > > Add entity change notification to Atlas based using the existing > atlas-notification module. > > > Diffs > ----- > > notification/pom.xml 2e12520 > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 7b3cf89 > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotification.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProvider.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationImpl.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java > 735655c > > notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProviderTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationConsumerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationImplTest.java > PRE-CREATION > repository/pom.xml 89d848c > repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java > 34f2ba3 > > repository/src/main/java/org/apache/atlas/listener/EntityChangeListener.java > 6465e92 > > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java > ea39f92 > > repository/src/main/java/org/apache/atlas/services/NotificationEntityChangeListener.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/TestRepositoryMetadataModuleFactory.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/discovery/HiveLineageServiceTest.java > db51ae5 > > repository/src/test/java/org/apache/atlas/repository/graph/GraphBackedMetadataRepositoryTest.java > bec3067 > > repository/src/test/java/org/apache/atlas/service/DefaultMetadataServiceTest.java > 3c22815 > > repository/src/test/java/org/apache/atlas/services/DefaultMetadataServiceTest.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/EntityImpl.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/IdImpl.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/TraitImpl.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/api/Entity.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/api/Id.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/api/Trait.java > PRE-CREATION > typesystem/src/test/java/org/apache/atlas/typesystem/EntityImplTest.java > PRE-CREATION > typesystem/src/test/java/org/apache/atlas/typesystem/IdImplTest.java > PRE-CREATION > typesystem/src/test/java/org/apache/atlas/typesystem/TraitImplTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/38341/diff/ > > > Testing > ------- > > mvn clean test > > All existing tests pass. > > New unit tests added. > > > Thanks, > > Tom Beerbower > >
