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="===============8300242558565772786=="
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: Tue, 15 Sep 2015 21:46:53 -0000
Message-ID: <20150915214653.9550.68039@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>

--===============8300242558565772786==
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?

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


--===============8300242558565772786==--