On Wed, Sep 6, 2017 at 1:59 PM, Ian Boston <[email protected]> wrote: > Hi, > Thanks for looking at them. > > On 6 September 2017 at 12:32, Francesco Mari <[email protected]> > 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 ?
It is OAK-6575-2. > 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 rest of the team will surely give an opinion on the original approach and the OAK-6575-1 and OAK-6575-2 variants. We will go with whatever the majority prefers. >> 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 understand the concerns. I wonder how many conversions we need to implement, and how often we will introduce new conversions. > 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 <[email protected]> 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 <[email protected]> wrote: >> > >> >> Hi, >> >> >> >> On 5 September 2017 at 07:55, Francesco Mari <[email protected]> >> >> wrote: >> >> >> >>> On Mon, Sep 4, 2017 at 6:18 PM, Ian Boston <[email protected]> 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 >> >> >> >> >>
