eolivelli commented on issue #510: Issue-605 BP-15 New CreateLedger API URL: https://github.com/apache/bookkeeper/pull/510#issuecomment-333878714 @sijie thank you very much for all of your comments and your time I think I have addressed all of your comments, there was a lot of things to be done I hope I did not miss something. There is still a bunch of issues I did not solve and require discussion/approval: 1) the BookKeeperBuilder interface and how to configure components In the final version we have explicit components. For me is OK, please validate 2) using BKException in methods which are in the legacy API (like 'close') What is your opinion ? Dropping BKException will keep binary compatibility, but break "source compatibility" I am OK with dropping it 3) IOException in BookKeeper main constructor I think the best would be to introduce a new BKException subclass like 'BKClientCreationException" and get rid of explicit IOException, which actually means nothing because it can be throw by serveral subsystems (ZK client, PlacementPolicy....) 4) Using Mockito and a new Utility class It is not very clear to me how to do better then this final proposal, I have added two new utility methods to MockBookKeeperTestCase since your did your review 5) Checkstyle I found checkstyle was not present in bookkeeper-server module. After this patch we will have checkstyle in new api package, other packages do not work for me. I really hope I did not miss other comments/issues ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
With regards, Apache Git Services
