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="===============4459236116723041690=="
MIME-Version: 1.0
Subject: Re: Review Request 38341: Provide Entity Change Notification
From: "Shwetha GS" <sshivalingamurthy@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 06:32:31 -0000
Message-ID: <20150916063231.9550.16053@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Shwetha GS" <noreply@reviews.apache.org>
References: <20150914125516.3774.88390@reviews.apache.org>
In-Reply-To: <20150914125516.3774.88390@reviews.apache.org>
Reply-To: "Shwetha GS" <sshivalingamurthy@hortonworks.com>

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


> On Sept. 14, 2015, 12:55 p.m., Shwetha GS wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/Entity.java, line 25
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071265#file1071265line25>
> >
> >     Users are already exposed to the type system and its classes. Referenceable in this case. We can use that itself instead of adding another interface
> 
> Tom Beerbower wrote:
>     It wasn’t clear to me that IReferenceableInstance was supposed to be end user facing.  It looks like it’s heavily used internally.  I think that having the notification Entity interface shields the notification users from any changes that we may need to make to the type system in the future.  Also, it allows us to expose things in a different way if so required by the notification users.  One example is the case where the Ranger guys want to see which values of a entity’s trait belong to the trait’s super types.  You can’t get that through IReferenceableInstance and IStruct.  It requires an additional call or calls to the type system to get the trait hierarchy.  If we are trying to make the system easy to use, that’s additional work that we can do for them.

IReferenceableInstance is internal to atlas. Referenceable is exposed to users. 

There is an API to get type definition if they want to know the hierarchy. I don't think they need to know which attribute belongs to which trait, we should discuss their usecase to check if they are using hierarchy correctly. If they really need it, its probably what even other users want, and we can add to Struct(instead of maintaining another interface)


> On Sept. 14, 2015, 12:55 p.m., Shwetha GS wrote:
> > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java, line 469
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071277#file1071277line469>
> >
> >     Move to another file
> 
> Tom Beerbower wrote:
>     Could you explain why?  The listener implementation is internal to DefaultMetadataService.  It's not ever intended to be used outside of this class.

Listeners are not part of DefaultMetadataService, for example, I might have a cache which listens to enity changes and provides faster access to entities. Its not the responsibility of DefaultMetadataService and hence its a listener and not hard coded in DefaultMetadataService. Same goes with entity notification


- Shwetha


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


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


--===============4459236116723041690==--