atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shwetha GS" <sshivalingamur...@hortonworks.com>
Subject Re: Review Request 38341: Provide Entity Change Notification
Date Mon, 12 Oct 2015 23:03:29 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message