atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tom Beerbower" <tbeerbo...@hortonworks.com>
Subject Re: Review Request 38341: Provide Entity Change Notification
Date Wed, 16 Sep 2015 19:51:56 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.
> 
> Shwetha GS wrote:
>     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)

Referenceable is a class.  Shouldn't we be exposing an interface?

Also, it seems strange to me that a method like getEntity(String guid) returns something called
Referenceable and not Entity.

Could we do something like this? ...

    package org.apache.atlas.typesystem.types;
    public interface Entity extends IReferenceableInstance, IStruct {...}

    public class Referenceable extends Struct implements Entity {...}

Then at least the public interfaces would look more natural ...

    public Entity getEntity(String guid)
    
and the implementation would be hidden.

Yes, there is an API to get type definition.  I was hoping to make this easier to use so that
they wouldn't have to make those additional calls.  Yes, they do need to know which attribute
is defined for which trait.  It is something that they specifically asked for.  I'll explain
...

Suppose the trait SuperTag defines a single attribute A.  The trait SubTag defines a single
attribute B.  SubTag has SuperTag as a super type, so it inherits the attribute A.
Ranger does not have the notion of a Tag hierarchy so for them SubTag is a Tag with attributes
A and B; SuperTag is an unrelated Tag with attribute A.

If an entity is associated with the trait SubTag, it is also associated with SuperTag through
inheritance.

So, for Ranger they need to know that the Tag attribute A for an entity applies to both Tag
SubTag as well as SuperTag.

Yes they can get the type information through the APIs but they don’t currently support
a Tag hierarchy.  Without that they need use API calls to figure out all of the Tag super
types each time they get an entity notification.  If they did support Tag hierarchy then they
could load the type information on startup and then listen for type change notification (which
we don’t currently have).

It sounds like you are in favor of keeping the client interfaces as minimal as possible which
I agree is a good thing in general.  However, I think that needs to be balanced with the requirements
of the user and making the interfaces easily usable.  If we can package everything that they
need in a generic way in a single notification instead of forcing them to make additional
calls then I think that we should consider it.


> 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.
> 
> Shwetha GS wrote:
>     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

I'm not sure I understand.  The functionality that I added to DefaultMetadataService was that
it provides event notification.  The fact that it uses an EntityChangeListener to do that
is an implementation detail.  I could have just as easily added a few private methods on the
DefaultMetadataService class and called them directly.

Are you saying that the EntityNotificationListener should be available for other MetadataService
implementations that might want to provide the same functionality?  If so, should the registration
methods for the listeners be part of the MetadataService interface?  As it is now, it looks
like EntityChangeListeners are only usable for a DefaultMetadataService.

If you prefer it in a seperate class then I'll make the change.


> On Sept. 14, 2015, 12:55 p.m., Shwetha GS wrote:
> > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java,
line 473
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071277#file1071277line473>
> >
> >     Everywhere else, jettison json is used. Lets re-use it
> 
> Tom Beerbower wrote:
>     In that code I just want to marshal a Java object to JSON.  I'm not sure how to do
that easily in Jettison.  Can you tell me how?  See this...  http://stackoverflow.com/questions/15426641/how-to-marshall-pojo-to-json-using-jettison

Looking at EntityResource to see how to do it with Jettison.

I see this ...

    final String entityDefinition = metadataService.getEntityDefinition(guid);
    JSONObject response = new JSONObject();
    ...
    if (entityDefinition != null) {
        response.put(AtlasClient.DEFINITION, entityDefinition);
        status = Response.Status.OK;
    }

So, we are not actually serializing the entity definition for the JSONObject but simply attaching
it is as a String.  It seems strange to return JSON as a string through the REST API.


- Tom


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