HSEARCH-5107 Add a test checking segment files on various index operations

[marko-bekhta]

https://hibernate.atlassian.net/browse/HSEARCH-5107
https://hibernate.atlassian.net/browse/HSEARCH-5108

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[yrodiere]

Back to this... we discussed it in a video call, but I don't remember exactly what your problem was. It was testing, right? I assume the tests described in the Jira issue are impossible to implement.

IMO:

1. For https://hibernate.atlassian.net/browse/HSEARCH-5107 , set up a test where you run a search, check that Hibernate Search's index reader is still open, then call merge(), then check that Hibernate Search's index reader is no longer open. It's imperfect as it won't check that the reader closing happens before the merging, but still, it's something.
2. For https://hibernate.atlassian.net/browse/HSEARCH-5108 , set up a test where you run a search, check that Hibernate Search's index reader is still open, then call refresh(), then check that Hibernate Search's index reader is no longer open.

OR just forget about integration tests and add appropriate unit tests -- I think we have a few already for the low-level Lucene stuff:

1. For https://hibernate.atlassian.net/browse/HSEARCH-5107 , just check that the merge() operation triggers an attempt to close readers before it does its thing.
2. For https://hibernate.atlassian.net/browse/HSEARCH-5108 , just check that the refresh() operation triggers an attempt to close readers before it does its thing.

[marko-bekhta]

what your problem was.

I'd say ................ the main problem is that I can't reproduce the problem 😅 ( the one described in the discourse discussion).

My understanding is that there was a problem: a reader is used by something, let's say - a query scrolling the results. and at that same time the merge operation is requested, resulting in segment files being merged, but the outdated stale segments locked by the reader were never removed.

I was trying to get that, but even with what we have right now (without closing anything), everything seems to work just fine. see this test for example:

hibernate-search/integrationtest/backend/lucene/src/test/java/org/hibernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java

Lines 57 to 83 in a380bba

	void merge() throws Exception {
	basicTestSteps( () -> {
	// now let's start scrolling so that we are going to lock some of the to-be-stale segments:
	SearchScroll<DocumentReference> scroll = index.query().where( f -> f.matchAll() ).scroll( 5 );
	SearchScrollResult<DocumentReference> next = scroll.next();
	assertThat( next.hits() ).hasSize( 5 );
	IndexWorkspace workspace = index.createWorkspace();
	workspace.mergeSegments( OperationSubmitter.blocking(), UnsupportedOperationBehavior.FAIL ).join();
	// we've merged the segments, so we expect that there's exactly 1 current segment
	assertThat( numberOfCurrentSegments() ).isEqualTo( 1 );
	// we do have a scroll opened so there are some stale files:
	assertThat( numberOfAllSegments() ).isGreaterThan( 1 );
	// scroll still works:
	next = scroll.next();
	assertThat( next.hits() ).hasSize( 5 );
	// now we are going to close the scroll releasing any stale files:
	scroll.close();
	assertThat( numberOfCurrentSegments() ).isEqualTo( 1 );
	Awaitility.await()
	.timeout( 10, TimeUnit.SECONDS )
	.untilAsserted( () -> assertThat( numberOfAllSegments() ).isEqualTo( 1 ) );
	} );
	}

The idea there was:

• create some initial docs and stop Search
  • that should generate some segment files
  • stopping Search should close any readers 😃
• then start Search with that index
• hold a reader with a scroll query
• merge segments
• check what's happening with the segment files on the disc
  • since we've merged, there should be only a single current segment file
  • since the scroll is holding the reader, a stale segment should also be there somewhere
• stop scrolling, should release any readers
• check what's happening with the segments:
  • a number of current segments is still 1
  • a number of all segments should become 1 as the reader was released and should get closed nicely.

I thiiiiiiink that:

• merge/purge tests in that same class is an attempt to do the integration test on the scenario I've described above
• this test here

  hibernate-search/integrationtest/backend/lucene/src/test/java/org/hibernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java

  Lines 126 to 139 in a380bba

  	basicTestSteps( () -> {
  	LuceneIndexManagerImpl luceneIndexManager = index.unwrapForTests( LuceneIndexManagerImpl.class );
  	assertThat( luceneIndexManager.getShardsForTests() ).hasSize( 1 );
  	Shard shard = luceneIndexManager.getShardsForTests().get( 0 );
  	IndexReader indexReaderBefore = shard.indexAccessorForTests().getCurrentReaderForTests();
  	assertThat( indexReaderBefore ).isNotNull();
  	IndexWorkspace workspace = index.createWorkspace();
  	operation.accept( workspace );
  	assertThat( shard.indexAccessorForTests().getCurrentReaderForTests() ).isNull();
  	assertThat( index.query().select().where( f -> f.matchAll() ).fetchTotalHitCount() ).isPositive();
  	assertThat( shard.indexAccessorForTests().getCurrentReaderForTests() ).isNotNull().isNotSameAs( indexReaderBefore );

  is close to your suggestion 1
• As for suggestion 2, I think you are talking about the IndexAccessorTest; which mostly covers the scenario here
  https://github.com/hibernate/hibernate-search/blob/a380bbac912bb581fba68511cda154b43bda9c55/backend/lucene/src/test/java/org/hibernate/search/backend/lucene/lowlevel/index/impl/IndexAccessorTest.java#L157C1-L165C3

[yrodiere]

I was trying to get that, but even with what we have right now (without closing anything), everything seems to work just fine. see this test for example:

I'd certainly expect things to work fine right now on Linux; this is a Windows-specific problem. On Linux (at last in every case I've encountered) you can delete a file that's still being read -- on Windows (at least in some cases) this will fail.

If it works on Windows, well... I don't know, I'd have to debug to find out how the merge manages to delete an open file, and I really don't want to :)

[yrodiere]

* this test here https://github.com/hibernate/hibernate-search/blob/a380bbac912bb581fba68511cda154b43bda9c55/integrationtest/backend/lucene/src/test/java/org/hibernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java#L126-L139
   is close to your suggestion 1

Agreed; added a few comments below.

* As for suggestion 2, I think you are talking about the IndexAccessorTest; which mostly covers the scenario here
  https://github.com/hibernate/hibernate-search/blob/a380bbac912bb581fba68511cda154b43bda9c55/backend/lucene/src/test/java/org/hibernate/search/backend/lucene/lowlevel/index/impl/IndexAccessorTest.java#L157C1-L165C3

Not quite, as it doesn't check that IndexReaderProvider will actually close readers immediately on calls to refresh. But, you check that in the integration test above, so you're right, this is enough.

[yrodiere]

Thanks. Feel free to merge, but see my comment

[sonarcloud]

Quality Gate failed

Failed conditions
78.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud