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