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