> 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 > > Tom Beerbower wrote: > 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?
That should error. Assuming default causes some messages to be missed if there is another consumer with the same group id and its difficult to debug > 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 > > Tom Beerbower wrote: > Are you saying that this method should allow for multiple entity > notifications and batch them together in one call to > NotificationInterface.send()? Yes, latency can be reduced that way. send() already supports multiple messages > 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? > > Tom Beerbower wrote: > 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? Yes, there is no HookNotification currently. You can assume String for now Yes, can be done as part of another jira. Create one and take it up after this > 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? > > Tom Beerbower wrote: > 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. No one reads docs, instead interface should be more clear. Provide getTrait() - Shwetha ----------------------------------------------------------- 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 > >
