ISPN-14127 Support non blocking guarantees on indexing APIs #10305
Conversation
fax4ever
force-pushed
the
ISPN-14127
branch
from
6ab5714
to
06750e6
Compare
fax4ever
force-pushed
the
ISPN-14127
branch
from
06750e6
to
dc53b9c
Compare
| this.indexProcessor = UnicastProcessor.<IndexingOperation>create().toSerialized(); | ||
|
|
||
| indexProcessor | ||
| .observeOn(Schedulers.from(blockingManager.asExecutor("search-indexer"))) |
There was a problem hiding this comment.
I didn't put this on my original branch to you. But it would be a ton better if we could have some sort of lifecycle that shuts down the processor (invokes onComplete). But I don't know how hard that is to add.
There was a problem hiding this comment.
Sure we can do that!
|
|
||
| /** | ||
| * @author Fabio Massimo Ercoli | ||
| */ | ||
| public class SearchIndexerImpl implements SearchIndexer { | ||
|
|
||
| private static final Log log = LoggerFactory.make(Log.class, MethodHandles.lookup()); |
There was a problem hiding this comment.
Shouldn't we use the Log impls provided from ISPN? Or is this just a query thing that it uses the Hibernate ones?
There was a problem hiding this comment.
For the search integration operations we usually use the org.infinispan.search.mapper.log.impl,Log, that is an Infinispan log of the search mapper submodule of the query module.
The reason for the existence of this log impl class is the fact that when we did the integration with Hibernate Search 6 we had an Infinispan Search Mapper module that was merged with the query module, but the Log impl files weren't.
| @@ -40,4 +40,8 @@ public interface Log extends BasicLogger { | |||
| SearchException invalidEntitySuperType(String entityName, | |||
| @FormatWith(ClassFormatter.class) Class<?> expectedSuperType, | |||
| @FormatWith(ClassFormatter.class) Class<?> actualJavaType); | |||
|
|
|||
| @Message(id = 14507, value = "Error processing indexing operation.") | |||
There was a problem hiding this comment.
This needs to be at least an ERROR. If this ever occurs, indexing will no longer work so it needs to be a prominent message.
| @@ -47,9 +67,9 @@ public CompletableFuture<?> addOrUpdate(Object providedId, String routingKey, Ob | |||
| return CompletableFuture.completedFuture(null); | |||
There was a problem hiding this comment.
I missed this before, all of these references should be replaced with CompletableFutures.completedNull()
|
|
||
| /** | ||
| * Test the parsing and transformation of Ickle queries to Lucene queries. | ||
| * | ||
| * @author anistor@redhat.com | ||
| * @since 9.0 | ||
| */ | ||
| public class LuceneTransformationTest { | ||
| public class LuceneTransformationTest extends SingleCacheManagerTest { |
There was a problem hiding this comment.
I ran this test with the changes and the SearchIndexImpl was never hit it? Is this test supposed to do any indexing via SearchIndexImpl?
There was a problem hiding this comment.
Good question. It is possible, if no indexing operations are executed. I'm checking it.
There was a problem hiding this comment.
I checked. The SearchIndexImpl is created when the search mapping is created for the test and even if we're not going to index anything the constructor requires the managers to init the flow processor, I think
|
--- closed the wrong PR --- |
fax4ever
force-pushed
the
ISPN-14127
branch
from
9d898b4
to
ac1baec
Compare
|
@wburns thanks again for the review! I updated the PR with the related changes |
|
Looks good to me. I did notice there is a leaked thread failure now due to it though and possibly a mass indexer one? |
I think that we still sometimes have this issue with the current master. |
|
Integrated into main, thanks @fax4ever ! |
https://issues.redhat.com/browse/ISPN-14127