You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Tom Beerbower <tb...@hortonworks.com> on 2015/09/14 06:08:49 UTC
Review Request 38341: Provide Entity Change Notification
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/
-----------------------------------------------------------
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
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > Looks good.
> > Not really an issue with this patch as much as the existing code, I am a little concerned about the use of a single exception type "AtlasException" everywhere.
> > Using a single exception type results in the loss of context when an exception occurs and will likely result in messy, incomplete or even incorrect error handling in the future.
> > This was an issue in Ambari and would we should start thinking about this early on in this project before we get to a point where we can't change it.
Thanks for the review!
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java, line 62
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071266#file1071266line62>
> >
> > description missing regarding when this exception will be thrown
In the cases where I'm throwing an AtlasException, I'm just rethrowing from another atlas method that's being called. I think that in all of those cases the description was missing from the other class... so I'm not really sure of the conditions that will trigger the exception. Sorry, that's sort of a lame answer but I think that we need to clean up some of the docs in the underlying atlas code first.
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java, line 136
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071266#file1071266line136>
> >
> > Consider using parameterized logging args which would allow for the removal of the if(isLevel) check.
> >
> > LOG.debug("Logging event for entity : guid={} : operation={} : values={}", entity.getId(), operationType, values);
Yes, good point. Done.
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java, line 153
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071266#file1071266line153>
> >
> > default block with log message?
The switch contains all the possible cases for that enum value. Do I really need a default?
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityImpl.java, line 83
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071268#file1071268line83>
> >
> > Could any of these fields be null? If not, the constructor doesn't enforce this invariant.
No, null should not be allowed. Good point ... I'll assert not null in the constructor.
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/EntityImpl.java, line 90
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071268#file1071268line90>
> >
> > could any of these fields be null?
no
> On Sept. 14, 2015, 3:15 p.m., John Speidel wrote:
> > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java, line 601
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071277#file1071277line601>
> >
> > seems odd that an exception is thrown from this method?
Again, it's thrown from a method called by this method. So, I can either rethrow it or just log it.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98843
-----------------------------------------------------------
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98843
-----------------------------------------------------------
Ship it!
Looks good.
Not really an issue with this patch as much as the existing code, I am a little concerned about the use of a single exception type "AtlasException" everywhere.
Using a single exception type results in the loss of context when an exception occurs and will likely result in messy, incomplete or even incorrect error handling in the future.
This was an issue in Ambari and would we should start thinking about this early on in this project before we get to a point where we can't change it.
notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java (line 62)
<https://reviews.apache.org/r/38341/#comment155457>
description missing regarding when this exception will be thrown
notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java (line 136)
<https://reviews.apache.org/r/38341/#comment155459>
Consider using parameterized logging args which would allow for the removal of the if(isLevel) check.
LOG.debug("Logging event for entity : guid={} : operation={} : values={}", entity.getId(), operationType, values);
notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java (line 153)
<https://reviews.apache.org/r/38341/#comment155460>
default block with log message?
notification/src/main/java/org/apache/atlas/notification/entity/EntityImpl.java (line 83)
<https://reviews.apache.org/r/38341/#comment155461>
Could any of these fields be null? If not, the constructor doesn't enforce this invariant.
notification/src/main/java/org/apache/atlas/notification/entity/EntityImpl.java (line 90)
<https://reviews.apache.org/r/38341/#comment155462>
could any of these fields be null?
repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 601)
<https://reviews.apache.org/r/38341/#comment155465>
seems odd that an exception is thrown from this method?
- John Speidel
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Sept. 15, 2015, 6:44 a.m., Suma Shivaprasad wrote:
> >
Thanks for the review!
> On Sept. 15, 2015, 6:44 a.m., Suma Shivaprasad wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/Entity.java, line 26
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071265#file1071265line26>
> >
> > Already exposed by Referenceable like Shwetha mentioned
Same as above reply to Shwetha ...
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.
> On Sept. 15, 2015, 6:44 a.m., Suma Shivaprasad wrote:
> > notification/src/main/java/org/apache/atlas/notification/entity/Trait.java, line 25
> > <https://reviews.apache.org/r/38341/diff/1/?file=1071270#file1071270line25>
> >
> > THis can also be removed to use existing interfaces
Same as above reply to Shwetha ...
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98993
-----------------------------------------------------------
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Suma Shivaprasad <su...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98993
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/notification/entity/Entity.java (line 26)
<https://reviews.apache.org/r/38341/#comment155779>
Already exposed by Referenceable like Shwetha mentioned
notification/src/main/java/org/apache/atlas/notification/entity/Trait.java (line 25)
<https://reviews.apache.org/r/38341/#comment155780>
THis can also be removed to use existing interfaces
- Suma Shivaprasad
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Sept. 24, 2015, 10:24 p.m., Seetharam Venkatesh wrote:
> > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java, line 250
> > <https://reviews.apache.org/r/38341/diff/2/?file=1081953#file1081953line250>
> >
> > It would be much cleaner to add a [Entity/Type]ChangeListener for this and avoid duplication of code.
>
> Tom Beerbower wrote:
> Thanks for the review. I used the listener approach initially but decided it was less code and basically the same thing to jusr use a private method since it seems that EntityChangeListener use is pretty much restricted to the DefaultMetadataService anyway. Since it's all internal to the DefaultMetadataService class, it's pretty much just an implementation detail to me. I can see how it might be useful to have a public EntityChangeListener for notifications if we ever exposed the listener registration outside of the DefaultMetadataService class, which is probably intended at some point I guess. I'll make the change to use a listener. Thanks.
Since this is internal and may include additional changes to existing interfaces (MetadataService and EntityChangeListener), I'd like to handle this as part of another merge. I'll open another Jira to track as part of the same sprint.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review100487
-----------------------------------------------------------
On Sept. 25, 2015, 2:13 p.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38341/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 2:13 p.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/pom.xml 2e12520
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotification.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumer.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProvider.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationImpl.java PRE-CREATION
> notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProviderTest.java PRE-CREATION
> notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationConsumerTest.java PRE-CREATION
> notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationImplTest.java PRE-CREATION
> repository/pom.xml 8e4d0f3
> repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java 34f2ba3
> repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java ea39f92
> repository/src/test/java/org/apache/atlas/TestRepositoryMetadataModuleFactory.java PRE-CREATION
> repository/src/test/java/org/apache/atlas/discovery/HiveLineageServiceTest.java db51ae5
> repository/src/test/java/org/apache/atlas/repository/graph/GraphBackedMetadataRepositoryTest.java bec3067
> repository/src/test/java/org/apache/atlas/service/DefaultMetadataServiceTest.java 3c22815
> repository/src/test/java/org/apache/atlas/services/DefaultMetadataServiceTest.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/EntityImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/IdImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/TraitImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Entity.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Id.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Trait.java PRE-CREATION
> typesystem/src/test/java/org/apache/atlas/typesystem/EntityImplTest.java PRE-CREATION
> typesystem/src/test/java/org/apache/atlas/typesystem/IdImplTest.java PRE-CREATION
> typesystem/src/test/java/org/apache/atlas/typesystem/TraitImplTest.java PRE-CREATION
>
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Seetharam Venkatesh <ve...@innerzeal.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review100487
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 250)
<https://reviews.apache.org/r/38341/#comment157692>
It would be much cleaner to add a [Entity/Type]ChangeListener for this and avoid duplication of code.
- Seetharam Venkatesh
On Sept. 23, 2015, 3:58 a.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38341/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 3:58 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/EntityNotification.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumer.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProvider.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationImpl.java PRE-CREATION
> repository/pom.xml 8e4d0f3
> repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java fbd01de
> repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java 56168db
> typesystem/src/main/java/org/apache/atlas/typesystem/EntityImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/IdImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/TraitImpl.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Entity.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Id.java PRE-CREATION
> typesystem/src/main/java/org/apache/atlas/typesystem/api/Trait.java PRE-CREATION
>
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/
-----------------------------------------------------------
(Updated Sept. 23, 2015, 3:58 a.m.)
Review request for atlas, John Speidel and Shwetha GS.
Changes
-------
Updated diff
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 (updated)
-----
notification/src/main/java/org/apache/atlas/notification/entity/EntityNotification.java PRE-CREATION
notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumer.java PRE-CREATION
notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationConsumerProvider.java PRE-CREATION
notification/src/main/java/org/apache/atlas/notification/entity/EntityNotificationImpl.java PRE-CREATION
repository/pom.xml 8e4d0f3
repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java fbd01de
repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java 56168db
typesystem/src/main/java/org/apache/atlas/typesystem/EntityImpl.java PRE-CREATION
typesystem/src/main/java/org/apache/atlas/typesystem/IdImpl.java PRE-CREATION
typesystem/src/main/java/org/apache/atlas/typesystem/TraitImpl.java PRE-CREATION
typesystem/src/main/java/org/apache/atlas/typesystem/api/Entity.java PRE-CREATION
typesystem/src/main/java/org/apache/atlas/typesystem/api/Id.java PRE-CREATION
typesystem/src/main/java/org/apache/atlas/typesystem/api/Trait.java PRE-CREATION
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
Re: Review Request 38341: Provide Entity Change Notification
Posted by Shwetha GS <ss...@hortonworks.com>.
> 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?
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.
- Shwetha
-----------------------------------------------------------
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Shwetha GS <ss...@hortonworks.com>.
> 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.
>
> Tom Beerbower wrote:
> 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 Beerbower wrote:
> Okay, I think that I see now. It's flipped from the entity notification... the hook is the sender and the server is the consumer.
>
> I don't see any usages of NotificationHookConsumer? Can you explain how it will be used (where is start called)?
NotificationHookConsumer is a service thats started and stopped with atlas start and stop. Its part of ATLAS-58 patch
- Shwetha
-----------------------------------------------------------
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> 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.
>
> Tom Beerbower wrote:
> 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.
Okay, I think that I see now. It's flipped from the entity notification... the hook is the sender and the server is the consumer.
I don't see any usages of NotificationHookConsumer? Can you explain how it will be used (where is start called)?
- 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Shwetha GS <ss...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98990
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/notification/entity/EntityChangeConsumer.java (line 41)
<https://reviews.apache.org/r/38341/#comment155778>
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?
- Shwetha GS
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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Sept. 14, 2015, 12:55 p.m., Shwetha GS wrote:
> >
Thanks for the review!
> 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
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.
> 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
Could you explain why? The listener implementation is internal to DefaultMetadataService. It's not ever intended to be used outside of this class.
> 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
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
- 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Shwetha GS <ss...@hortonworks.com>.
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Tom Beerbower <tb...@hortonworks.com>.
> 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
>
>
Re: Review Request 38341: Provide Entity Change Notification
Posted by Shwetha GS <ss...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38341/#review98834
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/notification/entity/Entity.java (line 25)
<https://reviews.apache.org/r/38341/#comment155448>
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
repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 469)
<https://reviews.apache.org/r/38341/#comment155450>
Move to another file
repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 473)
<https://reviews.apache.org/r/38341/#comment155451>
Everywhere else, jettison json is used. Lets re-use it
- Shwetha GS
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
>
>