Mailing-List: contact dev-help@atlas.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@atlas.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============2838378415934675083=="
MIME-Version: 1.0
Subject: Re: Review Request 38341: Provide Entity Change Notification
From: "Tom Beerbower" <tbeerbower@hortonworks.com>
To: "John Speidel" <jspeidel@hortonworks.com>,
 "Shwetha GS" <sshivalingamurthy@hortonworks.com>
Cc: "atlas" <dev@atlas.incubator.apache.org>,
 "Tom Beerbower" <tbeerbower@hortonworks.com>
Date: Wed, 16 Sep 2015 20:14:28 -0000
Message-ID: <20150916201428.3773.79787@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Tom Beerbower" <noreply@reviews.apache.org>
References: <20150915054407.3774.18126@reviews.apache.org>
In-Reply-To: <20150915054407.3774.18126@reviews.apache.org>
Reply-To: "Tom Beerbower" <tbeerbower@hortonworks.com>

--===============2838378415934675083==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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

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


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


--===============2838378415934675083==--