-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HSEARCH-5107 Add a test checking segment files on various index operations #4142
HSEARCH-5107 Add a test checking segment files on various index operations #4142
Conversation
8f7ccb6
to
c59f44d
Compare
...bernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java
Show resolved
Hide resolved
c59f44d
to
a380bba
Compare
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
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:
OR just forget about integration tests and add appropriate unit tests -- I think we have a few already for the low-level Lucene stuff:
|
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: Lines 57 to 83 in a380bba
The idea there was:
I thiiiiiiink that:
|
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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.
...bernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java
Show resolved
Hide resolved
...bernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java
Show resolved
Hide resolved
.../main/java/org/hibernate/search/backend/lucene/lowlevel/reader/impl/IndexReaderProvider.java
Outdated
Show resolved
Hide resolved
a380bba
to
17279b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Feel free to merge, but see my comment
...bernate/search/integrationtest/backend/lucene/lowlevel/reader/LuceneIndexSegmentFilesIT.java
Show resolved
Hide resolved
17279b5
to
7f68909
Compare
Quality Gate failedFailed conditions |
https://hibernate.atlassian.net/browse/HSEARCH-5107
https://hibernate.atlassian.net/browse/HSEARCH-5108