On June 21, 2018 at 6:53:44 AM, Marcel Reutegger ([email protected]) wrote:
Hi Matt, New files in your pull request have a different format for the Apache License header. Can you please change them to match the format of existing source files? Yes - I believe I have fixed this now, let me know if I missed any. As mentioned in an offline conversion with you already, I'm a bit concerned of the impact this optional feature has on nearly all layers of Oak. SessionImpl implements HttpBinaryProvider, MutableRoot implements HttpBlobProvider, SegmentNodeStore implements HttpBlobProvider, DocumentNodeStore implements HttpBlobProvider. E.g. the last two just pass through calls they are not concerned with. Alternatively, could you do the required plumbing on construction time? That is, if the BlobStore implements HttpBlobProvider register it with that interface as well and use it to construct the repository. Something like: BlobStore bs = ... NodeStore ns = ... Jcr jcr = new Jcr(ns) if (bs instanceof HttpBlobProvider) jcr.with((HttpBlobProvider) bs) Repository r = jcr.createRepository() By default, the Jcr factory would have a HttpBlobProvider implementation that doesn't support the feature, which also relieves the repository implementation from checking the type or for null on every call to the new feature (as is the case in SessionImpl, MutableRoot, DocumentNodeStore, SegmentNodeStore). I added OAK-7570 to discuss this. I would also prefer if the API used by the client is moved to a separate module that can be release independently. Yes, we don't do this right now in Oak, but this may be a good opportunity to try this again. Releasing the API independently with a stable version lowers the barrier for consumers to adopt it. I added OAK-7571 to discuss this. -MR
