atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tom Beerbower" <tbeerbo...@hortonworks.com>
Subject Re: Review Request 38341: Provide Entity Change Notification
Date Tue, 15 Sep 2015 21:46:53 GMT


> On Sept. 15, 2015, 5:44 a.m., Shwetha GS wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java,
line 41
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071266#file1071266line41>
> >
> >     why expose interface using threadpool and listeners. Why not expose NotificationInterface
directly.
> >     
> >     Exposing through threadpool and listener creates following issues:
> >     1. Users may not be aware of threadpool created on client side. This may add
to debugging complexity. They may even create their own thread pool as well.
> >     2. If users have another way of parallelism or another thread pool of their
own, they can't combine both.
> >     3. Listeners are called synchronously. Users may not be ok with it
> >     
> >     
> >     In general, its better to provide thin clients to users so that they can implement
their own usecases their own way. That way, maintenance on our end will be less. Makes sense?

I see your point and don't disagree but there is no requirement for the user to use this class.
 With the code as it currently is, they can inject a NotificationInterface, create the consumers
themselves and manage their own threads.  My main intent with introducing this class was to
give the users an easy to use interface out of the box.  It really just saves them from having
to write the same code.  

Also, I basically copied most of the EntityChangeConsumer code from the existing NotificationHookConsumer
code, which does the same as far as threadpool, etc.  Don't your arguments apply there as
well?


- Tom


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98990
-----------------------------------------------------------


On Sept. 14, 2015, 4:08 a.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38341/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2015, 4:08 a.m.)
> 
> 
> Review request for atlas, John Speidel and Shwetha GS.
> 
> 
> Bugs: ATLAS-158
>     https://issues.apache.org/jira/browse/ATLAS-158
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add entity change notification to Atlas based using the existing atlas-notification module.
> 
> First cut at a patch for the Atlas entity change notification.  Note that at a minimum
additional unit tests are required.  I'm putting up the review to get some initial feedback.
> 
> 
> Diffs
> -----
> 
>   notification/src/main/java/org/apache/atlas/notification/entity/Entity.java PRE-CREATION

>   notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java
PRE-CREATION 
>   notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeListener.java
PRE-CREATION 
>   notification/src/main/java/org/apache/atlas/notification/entity/EntityImpl.java PRE-CREATION

>   notification/src/main/java/org/apache/atlas/notification/entity/EntityNotification.java
PRE-CREATION 
>   notification/src/main/java/org/apache/atlas/notification/entity/Trait.java PRE-CREATION

>   notification/src/main/java/org/apache/atlas/notification/entity/TraitImpl.java PRE-CREATION

>   notification/src/test/java/org/apache/atlas/notification/entity/EntityImplTest.java
PRE-CREATION 
>   notification/src/test/java/org/apache/atlas/notification/entity/TraitImplTest.java
PRE-CREATION 
>   repository/pom.xml 8e4d0f3 
>   repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java fbd01de 
>   repository/src/main/java/org/apache/atlas/listener/EntityChangeListener.java f58d6de

>   repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java 56168db

> 
> Diff: https://reviews.apache.org/r/38341/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test
> 
> All existing tests pass.
> 
> New unit tests added (more required).
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>


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