On 21.06.2018, at 05:53, Marcel Reutegger <[email protected]> wrote: > 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.
Yes, that is the case, but it's just because Oak has so many layers, so if you want to add a new proper API, it means adding it in a few places :) > 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. The design of Oak is explicitly that the NodeStore controls binaries, and use of a BlobStore is only optional. Root and Tree only see the NodeStore, not the BlobStore. The SegmentNodeStore even makes the choice to inline binaries under 16 KB for example. For these, the new HTTP access is not possible and getHttpDownloadURL() must return null in that case. This logic can only happen in the implementation of the NodeStores. Note that the API changes are designed to be fully backwards compatible through the use of new extension interfaces. I.e. Session might implement HttpBinaryProvider (and Oak's SessionImpl does in the patch) and clients have to do an instanceof check. Same as with JackrabbitSession and co. In no place did we add a method to an existing API interface. The reasons we added it as JCR API extension are: - existing client code using the JCR API does not need to change to an Oak API or have knowledge of the datastore; only needs to import the new extension interface (package o.a.j.oak.jcr.api.binary in oak-jcr, alternatively we could move it to jackrabbit-api (see below as well) - client code can access the feature from the Session object - permission check for upload (IMO critical) can only happen inside the SessionImpl [1] - converting Blob to Binary and vice versa is impossible outside of oak-jcr (unless we are missing something…) - NodeStore & Blob semantics as outlined above - no need for reflection tricks (as in the CDN feature discussed last year and moved out of Oak in the end) > 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). How would a client access it? How would the permission check be implemented? > 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. There are 3 new API/SPI additions: 1) oak-jcr: client facing: HttpBinaryProvider 2) oak-api: shared for Oak API, NodeStore and BlobStore SPIs: HttpBlobProvider 3) oak-blob-plugins: for Jackrabbit DataStore SPI: HttpDataRecordProvider We can move 1) to jackrabbit-api and 3) to jackrabbit-data. We left that open for the review - it was just a lot easier to work on the patch if it was all contained in the jackrabbit-oak repository. Still the implementation changes in all layers are necessary as per above. [1] https://github.com/apache/jackrabbit-oak/pull/88/files#diff-190f7c0d7156c8ab24c49208f9eb04f2R793 [2] https://wiki.apache.org/jackrabbit/Direct%20Binary%20Access#Oak_Layers Cheers, Alex
