Hi, Thanks for looking at them. On 6 September 2017 at 12:32, Francesco Mari <mari.france...@gmail.com> wrote:
> I personally prefer the second approach. Is that OAK-6575-1 or OAK-6575-2 ? I assume OAK-6575-1 since OAK-6575 was my first approach ? If you mean OAK-6575-2, then I think someone with more knowledge of Oak will need to do the work as I am not at all confident I have covered the potential class/method navigation between a OakValue and a DataStore.... or if that navigation is even possible where the exposed datastore might actually be a composite datastore with the exposed part having no class based connection with the underlying S3 DataStore. (eg S3 DS cache). I am definitely not proud of OAK-6575-2, imho it's not elegant or efficient and would put up more barriers to future agility rather than remove them. > The only thing I'm not sure > about is if we want to define OakConversionService with such a > wildcard method. Assuming that OakConversionService will be called > from code running on top of the JCR API, we could provide instead more > specific conversion methods. For example, > > URI toURI(javax.jcr.Binary binary); > > What do you think about it? Is it too restrictive? Do we need a > wildcard method like currently defined in OakConversionsService? > Originally OakConversionsService would not have needed a new version of the package for each new conversion Oak supported, greatly simplifying dependencies downstream, especially where the source and target classes already exist. If a concrete method is used, the package will need to be versioned everytime. I suspect OSGi rules will require a minor version number increment each time, which is going to make a downstream developers life painful. In addition if an implementation bundle in Oak decides it wants to optionally support a conversion, it wont need to version the Oak API to achieve that. With concrete methods, ever change, wherever they are and however experimental will require a new version of the Oak API. This was the reason for going for a wildcard method. It allows extension without any downstream disruption, missing dependencies or out of band dependencies. I think the boils down to how much disruption Oak wants to inflict downstream to get new capabilities added, or inversely, how open Oak is to requests for API changes from downstream ? > > Moreover, I would leave PrivateURI out of the picture for the moment > since it's not clear from the patch how this is supposed to be used. > In fact, a comment in S3Backend explicitly states that is not > supported at this time. > PrivateURI was discussed on the OAK-6575 thread. It was added to the patch to illustrate how each patch would cope with extension of a new type. I propose to drop it from the final patch, however, in the second patch the disruption is quite large so it might be worth leaving it in there so that it can be implemented without more Oak API version changes. Best Regards Ian > > Finally, I suspect that in the second patch there was too much of an > aggressive rename refactoring. "types" was renamed to "customtypes" in > a lot of unrelated places. I would definitely double-check that. > On Tue, Sep 5, 2017 at 2:09 PM, Ian Boston <i...@tfd.co.uk> wrote: > > Hi, > > > > Repeating the comment to on OAK-6575 here for further discussion. 2 new > > Patches exploring both options. > > > > https://github.com/ieb/jackrabbit-oak/compare/trunk.. > .ieb:OAK-6575-1?expand=1 > > > > This drops the OSGi AdapterManager/AdapterFactory in favour of a non OSGi > > static pattern. Implementations of the AdapterFactory self register > rather > > than rely on OSGi doing the wiring. This is probably an IoC anti pattern, > > but does avoid exposing the AdapterFactory/AdapterManager outside Oak. > > > > https://github.com/ieb/jackrabbit-oak/compare/trunk.. > .ieb:OAK-6575-2?expand=1 > > > > This drops the AdapterManager concept completely and attempts to get from > > Value to URI using mix in interfaces and instanceof. I cant be certain it > > manages to do this as there is a disconnect between Blob, Blobstore and > > DataStore implementations with no guarantee that a BlobStore as seen by > the > > Blob implementation actually implements DataStore, or the Blob that is > > exposed in the JCR Value (implemented by OakValue) actually connects to > the > > correct DataStore of it it connects to a FileDatastore cache on local > disk. > > I could only wire this as far as I did with API changes. I may have > broken > > some of the new multi node store and multi datastore code used for 0DT in > > the process. An Oak committer with global knowledge will probably be able > > to do better. > > > > > > > > On 5 September 2017 at 08:19, Ian Boston <i...@tfd.co.uk> wrote: > > > >> Hi, > >> > >> On 5 September 2017 at 07:55, Francesco Mari <mari.france...@gmail.com> > >> wrote: > >> > >>> On Mon, Sep 4, 2017 at 6:18 PM, Ian Boston <i...@tfd.co.uk> wrote: > >>> > Do you mean: > >>> > keep the OakConversionService but put all the logic to convert from > a > >>> > Value to a URI inside that implementation using new Oak SPI/APIs if > >>> > necessary and drop the AdapterManager completely ? > >>> > >>> Yes. I think there is no need to provide a generic adapter-like > >>> implementation to solve this use case. > >>> > >>> > This would mean something the datastore implementation implements > which > >>> > oak-core can navigate to would have to implement a mix in interface > >>> with a > >>> > getURI() method. I am not certain what or how without trying to do > it. > >>> > > >>> > Would that address your concern here ? > >>> > >>> I think it's worth trying. Thanks for bringing the conversation > forward. > >>> > >> > >> > >> I will create 2 new branches. > >> 1 with no adapter manager relying on mixin interfaces and one with a non > >> OSGi adapter manager plugin pattern. > >> > >> Thanks for the input. > >> Best Regards > >> Ian > >> > >> >
