[hibernate-dev] [Search] OSGi split packages

[Sanne Grinovero]

On 26 March 2014 20:18, Hardy Ferentschik <hardy at hibernate.org> wrote:
>
> On 26 Jan 2014, at 20:52, Sanne Grinovero <sanne at hibernate.org> wrote:
>
>> The classes in the ORM module are extremely widely used by our primary target:
>>
>> - FullTextQuery
>> - Search
>> - FullTextSession
>>
>> and two more lesser ones, but I think the 3 above are probably "THE
>> API”.
>
> That was my thinking as well.
>
>> To move these out of the way we'd at least need to postpone the
>> OSGi work for after a Beta to include a deprecated version of these.
>
> ? I don’t get what you are saying here. Deprecate it in an Alpha in order
> to then move them in a Beta release? That makes no sense what soever to me.
> My take on this is, that we are dealing with a new major version of Search
> and that we are still in Alpha phase. If we need to make non backwards
> compatible changes we should do it now.

If we need to change public API we have the obvious options:
 1# just break the API
 2# deprecate the old one

I didn't mean that we stricly should do 2#, but we should at least try hard ;-)
But *if* we go for 2#, we should keep the deprecated classes around
for at least a Beta release, or the effort (of keeping them) is not
worth it as I think an Alpha release is not going to inspire enough
confidence, or have enough visibility, for us to suggest using it as a
migration milestone.

Problem is, that if we keep them you might not be able to finish the
OSGi issue, as it requires these classes to be removed. Or maybe we
can relax this requirement temporarily? I guess the important
milestone to aim at is to define the final API? I guess we shouldn't
necessarily aim at fully resolving HSEARCH-1465 in a single tag, and
if it comes to breaking our APIs I'd prefer we consider alternatives
carefully.

>> Or we keep them around in the backwards compatible module
>> "hibernate-search” ?
>
> For what? Making things even more complicated?

I agree with you: I'm not suggesting we actually do it, I'm listing
our options: some I like, some I don't (like this one). You can
consider it a rethorical figure which stresses the fact the previous
alternative is better.

>
>> On the other option, in Engine we have:
>> - Environment (containing all the configuration constants)
>> - ProjectionConstants (more useful constants)
>> - SearchException
>> - SearchFactory (another strong case of public API)
>>
>> Also to remember the changes in Engine also affect the HQL parser
>> (OGM) and Infinispan Query and its users; we can adapt for it but
>> ultimately that's our responsibility too, probably best to verify
>> early on to see the impact. Not really a reason to not do it, but it
>> has an impact on several other projects which we need to facilitate.
>
> Mind you, we are talking just about an import statement change. No API
> or functional change. And yes, our downstream projects would need to adjust.

I get that. Still a nice self-documenting deprecated class is so much
easier to handle than an import statement change.


>> I think I like Hardy's approach of moving from Engine better; not
>> least we can provide alternative deprecated constants in the ORM
>> module for ORM users to use as a migration help (Environment and
>> ProjectionConstants).
>
> ?

You could make a copy of Environment and ProjectionConstants in the
ORM module, and deprecate it to document the new position. Maybe we
shouldn't invest too much into backwards compatibility, but since this
is a trivial step we should just do it.


>> To reply on the more fine-grained details:
>>
>>> org.hibernate.search.Environment        ->  org.hibernate.search.cfg.Environment
>>
>> since it's API, WDYT of a fully fledged mouthfull
>> “org.hibernate.search.configuration.Environment"
>
> I like it. In fact I wish cfg would have been called configuration in the first place.
> Having both, cfg and configuration seems odd to me. In this case I prefer to just
> use cfg

Same here. I almost suggested to rename all of _cfg_ to
_configuration_ but I didn't realize it contained so much public API,
so +1 to stick with cfg.

>>> org.hibernate.search.SearchFactory     ->  org.hibernate.search.spi.SearchFactory
>>
>> as Emmanuel said, that's an API currently.
>
> Right. I am not 100% happy with this choice either. Mainly moved it there, since we have SearchFactory
> related classes in there already (not all of which are api either)
>
>> I'm inclined to think we
>> should convert it to an SPI, and provide a richer more ORM specific
>> version in the ORM module?
>
> Interesting. On the other hand, making SearchFactory an spi sounds strange to me.

I meant conceptually, the integrators don't really need SearchFactory
at all: they rely on SearchFactoryIntegrator. In practice Infinispan
Query also exposes the SearchFactory as an API but they could expose
an alternative definition for the same functional role.

If we break this link, we have the added benefit of being able to
expose ORM-specific methods on the public API SearchFactory which we
would be bundling in the hibernate-search-orm package (rather than
engine as it's today). I don't say there is anything that comes to
mind urgently needing this, but it seems nice in terms of future
flexibility.

The tricky part is that today SearchFactoryIntegrator extends
SearchFactory, but since SearchFactoryIntegrator needs to say in
Engine, it can't depend on SearchFactory. What would you think of
breaking this parent-child relation?
>From a user's perspective they wouldn't be able to narrow down their
SearchFactory to an SearchFactoryIntegrator, but we could provide an
unwrap backdoor.

I'm actually thinking now that I'd like to make this change, even if
we decided to not move the location for SearchFactory: cleans up a bit
of the SearchFactoryIntegrator/SearchFactory/SearchFactoryImplementor/SearchFactoryImplementorWithShareableState
hierarchy which we have grown over time to accomodate some of these
integration needs.


>
>>> org.hibernate.search.SearchException ->  org.hibernate.search.exception.SearchException
>>
>> Maybe making the package plural? We should add variations for specific failures.
>
> +1 I was about to suggest that as well

ok

>>> org.hibernate.search.FullTextFilter        ->  org.hibernate.search.filter.FullTextFilter
>>
>> "filtering" ? "filters" ?
>> Not too fond of the singular term as it's not very explicit (yes I
>> know it's an existing one).
>
> I think I would stick with filter. I guess it comes down to how disruptive we want to be

ok

>>> org.hibernate.search.Version                ->  org.hibernate.search.engine.Version
>>
>> +1
>
> :-)

ok :-)

Hoping I didn't miss some crucial point, I think we actually have a
reasonable plan. Curious now to hear Emmanuel's POV.
Downsides summary: we'd be breaking significant public APIs of both
Hibernate Search and Infinispan Query, and also some SPIs :-/
The only reason I'm even thinking of this is that actually some of
these changes look like reasonable even out of the OSGi requirements
scope :-)

Sanne