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 Fri, 18 Sep 2015 05:58:19 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?
> 
> Tom Beerbower wrote:
>     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?
> 
> Shwetha GS wrote:
>     The problem with giving more in client libraries is that it goes as a contract and
it needs to be perfect, should work for all users, make sure that its backward compatible
in all future releases. So, more maintenance. Its better to give the bare minimum and let
users build on that. NotificationInterafce is not complicated, just create consumer and they
get an iterator on message. If you think thats complicated, simplify it.
>     
>     As for using the same in NotificationHookConsumer, NotificationHookConsumer is part
of atlas server. Currently, atlas server doesn't have a thread pool and hence we need it.
We can change it anytime without informing the users(not the admins who monitor the atlas
service). Its part of server, hence not all users need to debug this(only the admins) and
typically, all services will have thread pools.
> 
> Tom Beerbower wrote:
>     I'm not arguing that NotificationInterface is complicated.  I just thought that the
EntityChangeConsumer was in line with the work you had already done in NotificationHookConsumer
and was a bit easier to use than creating the consumers directly.  I'll remove EntityChangeConsumer.
>     
>     Aren't the other services that implement a hook just clients of Atlas hook notification,
just like Ranger is a client of Atlas entity notification?  Maybe I don't understand correctly
how the hooks work.
> 
> Tom Beerbower wrote:
>     Okay, I think that I see now.  It's flipped from the entity notification... the hook
is the sender and the server is the consumer.  
>     
>     I don't see any usages of NotificationHookConsumer?  Can you explain how it will
be used (where is start called)?

NotificationHookConsumer is a service thats started and stopped with atlas start and stop.
Its part of ATLAS-58 patch


- Shwetha


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