eolivelli commented on issue #510: Issue-605 BP-15 New CreateLedger API URL: https://github.com/apache/bookkeeper/pull/510#issuecomment-331845529 @sijie I have updated the patch with: - all your comments addressed (I hope) - new ByteBuffer API - new Testcases bases on Mockito I had to add several 'getters' to BookKeeper class in order to make it possible to create mocks. Essentially in order to mock the open of a ledeger (with fencing) I had to mock BookieClient, BookieWatcher, LedgerManager and other facilities. I did not create an 'Utility class' to create a "Mock" BookKeeper but a base MockBookKeeperTestCase, I am not an expert of Mockito at all but it was unhappy with using partially created mocks, so the testcases MUST have all the mocks as accessible variables. I we create a "Utils.createMockBookKeeper" the returned object won't be able to expose its internals in order to complete the creations of mocks. We have two options: 1) the one I have proposed (a MockBookKeeperTestCase) 2) a createMockBookKeeper which returns a structure which contains all the mocks Each of the two options have drawbacks Other point: The new test cases with Mockito are on the o.a.bookkeeper.client package and not o.a.bookkeeper.client.api package because in order to use Mockito I had to introduce several getters and I did not want to make them "public" Other point: I have left a single test case which uses new addEntry API (append/write). I think that anyway it is good to have at least one test files which really uses that API. I you fell strong that this is not necessary I will drop that class but I think it is unfortunate to publish a new API and not to have any testcase which exercises it ---------------------------------------------------------------- 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
