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