[hibernate-dev] "Service" in Hibernate Search: history, lessons learned and rewrite

[Sanne Grinovero]

I can't mention Service w/o getting your attention :)

On 27 April 2016 at 13:34, Hardy Ferentschik <hardy at hibernate.org> wrote:
> Hi,
>
>> Yet it is a good idea nowadays to have LuceneWorkSerializer extend
>> Service, as we dropped that level of flexibility. This implies that
>> there's a single type of serializer (at most)
>
> Ok
>
>> and it's totally fine to expose this as:
>>    SearchIntegrator#getLuceneWorkSerializer()
>
> How is that related to the service discussion and why do you want to offer it.
> What is the benefit? The SearchIntegrator already gives access to the ServiceManager
> from where the serializer can be retrieved. Why exposing an explicit getter?
> Are you going to add getters for all services? Would this getter do something
> different than accessing the service manager?

I hope the reply to Gunnar qualifies here too: it's indeed a thin line
but it's equally unclear when something should (not) be a Service.
There are some cases in which one can only user the Service, so these
would never able to get a Getter.

>
> On a tangent, the whole serialization framework would need an overhaul, especially
> the Serializer and Deserializer interfaces. They are very tightly coupled on how
> Avro works and tbh they are quite convoluted.

+1

I wouldn't mind allowing the "backendqueue" to make its own
arrangements, i.e. not mandate the contract anymore. We just need to
provide at least one implementation, but others - especially if not
interested in Avro - should be able to do something very different.

We're also working to remove the Apache Document as something exposed
to FieldBridge: by doing so, we could generate the actual Document
only at receive side, and not have to serialize Apache code anymore
but serialize an easier structure.

>
>> # A Service implementation can be provided by having it injected at
>> bootstrap (i.e.
>> org.hibernate.search.cfg.spi.SearchConfiguration.getProvidedServices()
>> )
>>
>> Not a new rule either: repeating for clarity. We call these "provided" services.
>
> One thing, I was never happy about was that provided services get a sopecial treatment
> in the service manager. They can returned first, provided they match the requested service
> type and they are don't get "released" (meaning they don't take part in the reference counting).
> I think services which are managed by the service manager should be treated equal.
> Maybe that becomes possible now that you consider dropping the reference counting, which
> I think is a good idea.

We need a way to have people override an internal component by handing
in an instance: this was the very reason to create this Service
manager.

An example, is people running many instances of Search in the same JVM
(many apps in an app server?) they can share some instances, provided
we identify some components worth it.
I can't think of such a component consuming enough memory to warrant
doing this in our "traditional" use case (paired up with Hibernate
ORM) but there are several heavy weight components which could be
shared in the Infinispan use case.



>> Currently this expects a single implementation to be available:
>> there's no way to pick which one if there are multiple implementations
>> on the classpath.
>
> We have been discussing to extend the implementation once the needs arises to
> allow multiple service implementation to be available on the classpath in which
> case the user must provide additional configuration to allow to select the correct
> implementation. So far there was no need to add this, but maybe now!?

There isn't specific need yet, I'm just pointing out to the newcomers
to be aware of this :)

>
>> Currently implementations are looked up "on demand". I plan to allow
>> "pre-initialize" services as it removes some trouble;
>
> What exactly does that mean? Didn't we have this discussion before?
> Which troubles are you talking about?
>
>> not least to remove the concurrency overhead.
>
> This argument could of course not be missed out ;-)
>
>> # If a Service implementation also implements Startable, or Stoppable,
>> we'll invoke the respective methods once at start and/or at stop of
>> the Search instance - unless they are provided in which case they are
>> ignored.
>
> This is something I never full understood. If a provided services does not
> want to get started/stopped would it then just not implement these interfaces?

Yes there are ways around, but it seems more practical to just allow it.
As mentioned in my other reply, it's not always easy to implement it
the "clean way" you suggest - not least if someone is skilled enough
to want to override components, should be able to figure the
limitations out.

>
>> The current javadoc suggests that it's illegal for a provided
>> implementation to also implement Startable and/or Stoppable; I don't
>> remember why that was, but today it seems unfitting
>
> Illegal might be the wrong term. It probably should say that it should not
> implement them if it does not want to have these callbacks invoked. Which is
> pretty much obvious. AFAIR, there was a discussion that we never should start/stop
> a provided service and hence this wording, but I think in code we never enforced it.

It currently enforces it, and we even have unit tests to make sure of it :)

>
>> The important concept which will survive, is that we don't start or
>> stop stuff which is provided as that's clearly responsibility of
>> another component.
>
> Then it should not implement Startable/Stopable and if it does, it should be
> noops.
>
>> # All non-provided Services will be stopped once, and only once as
>> final step when the SearchIntegrator is stopped.
>
> +1
>
>> This is a significant difference with today's code: we expect the
>> Service consumers to "open / close", hopefully in a finally block, to
>> the point that Gunnar enhanced it to at least allow AutoClose
>> semantics.
>
> I think this is still a good practice. I'd keep the API like this,
> but don't do reference counting and stop the service each time the
> count reaches 0. Instead, as you say, stop all services at a final
> shutdown.

Yea, I think the API is fitting.
The main problem I'm seeing is confusion about when it's appropriate
to make one..

The cases which I don't like the open/close is when some code blocks
need a service within the scope of a single method call.

As you say, long-lived components should grab a reference and reuse it
over time, then finally release it, but e.g. ClassLoaderService is
often needed "one shot" e.g. to setup a component.. which is were I
think a simple getter - which also bypasses reference counting - would
be more useful.

>
>> I don't plan to implement the hierarchical ServiceManager right now,
>> but proposing it already so that we can agree on the above cleanups in
>> contract, with the perspective that there are cleaner solutions also
>> for the scoped use case.
>
> What do you even understand under a hierarchical index manager?
>
>> Implementing these changes resolves or obsoletes at least 10 JIRA
>> issues in one shot..
>
> Really?

Ok, just 8 after closer inspection :)

>
> --Hardy
>