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