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 Wed, 16 Sep 2015 06:32:31 GMT


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


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