HSEARCH-1663 Packages not exported from osgi bundles

[gustavocoding]

https://hibernate.atlassian.net/browse/HSEARCH-1663

[Sanne]

Could you explain why Query is needing these exported?
Rather than exposing packages in ".impl" we might want to define a better SPI integration.

[hferentschik]

You are adding impl packages. How comes you need them? If you do, something seems fishy imo. I tried to avoid exporting impl packages intentionally in the OSGi metadata. A single case might be arguable, but these are a lot of impl classes. What exactly is the problem here? Why are they needed?

[Sanne]

Couple more things to consider:

• without a test or a comment, these "impl" exports might get "cleaned up" without us remembering about it
• we might rename/refactor classes and method names in such packages
• people will not just use Maven dependencies, we're expected to be able to maintaindrop-in compatibility across minor versions

[Sanne]

BTW good catch, as we wouldn't be able to fix the package structure if this came much later ;-)

[gustavocoding]

@Sanne I agree with you.
@hferentschik I agree with you too.

But unfortunalely what we have now is for example [1], [2] and [3], pieces relying on internal guts of search.

[1] https://github.com/infinispan/infinispan/blob/master/query/src/main/java/org/infinispan/query/indexmanager/LuceneWorkTransformationVisitor.java
[2] https://github.com/infinispan/infinispan/blob/master/query/src/main/java/org/infinispan/query/impl/massindex/IndexingMapper.java
[3] https://github.com/infinispan/infinispan/blob/master/query/src/main/java/org/infinispan/query/indexmanager/InfinispanIndexManager.java

[hferentschik]

Rather than exposing packages in ".impl" we might want to define a better SPI integration.

That's what we need to do. We need do determine what is really needed from the Infinispan Query side (and maybe what not) and potentially refine the api/spi.

[hferentschik]

Seems to me these are the culprits, right?

• org.hibernate.search.backend.impl.WorkVisitor
• org.hibernate.search.backend.impl.batch.DefaultBatchBackend
• org.hibernate.search.indexes.impl.DirectoryBasedIndexManager
• org.hibernate.search.infinispan.impl.InfinispanDirectoryProvider

[Sanne]

So an alternative solution would be to:

• promote org.hibernate.search.backend.impl.WorkVisitor to SPI
• an interface to shield you from directly needing org.hibernate.search.backend.impl.batch.DefaultBatchBackend
• make an SPI out of org.hibernate.search.indexes.impl.DirectoryBasedIndexManager
• make an SPI our of org.hibernate.search.infinispan.impl.InfinispanDirectoryProvider
• etc...

@gustavonalle maybe it would help to define the requirements by using CheckStyle in Infinispan Query to make it a violation to import packages under org.hibernate.*.impl*?

[Sanne]

@hferentschik +1, but also I notice now that some more .impl packages are being exported. Do we hae a JIRA to attempt to fix that?

[hferentschik]

Some thoughts:

If we end up exposing WorkVisitor, we should look at HSEARCH-1356 again. It's just not a visitor pattern.

What is this DefaultBatchBackend anyways? The docs says: "This is not meant to be used as a regular backend". So, if it should not be used, why is it used? Infinispan could also just have its own batch backend impl in this case.

Why would DirectoryBasedIndexManager and InfinispanDirectoryProvider be an SPI? They are not really extension points, but rather just classes which can be used. So imo we should promote them to API.

[Sanne]

Why would DirectoryBasedIndexManager and InfinispanDirectoryProvider be an SPI?

The plan is to hide Directory from public API or we won't ever be able to integrate nicely with remote indexing servers. Although obviously this might be needed for many integration points.

+1 to promote InfinispanDirectoryProvider as API

[Sanne]

What is this DefaultBatchBackend anyways? The docs says: "This is not meant to be used as a regular backend". So, if it should not be used, why is it used? Infinispan could also just have its own batch backend impl in this case.

It's a BATCH Backend, not a regular one. It's the one used by massindexer implementors (there are several).

Infinispan doesn't need to refer the the implementation, but it would need a way to construct one. We might need to introduce a factory on an SPI or a service.

[hferentschik]

+1 to promote InfinispanDirectoryProvider as API

cool

[hferentschik]

Infinispan doesn't need to refer the the implementation, but it would need a way to construct one. We might need to introduce a factory on an SPI or a service.

Ok, I guess we can sort out some details here.

[hferentschik]

Closing this pull request. I guess we agree that a different approach needs to be taken and it seems we have a solution for most problematic cases.