Aha! I realized that I was totally misreading some stacktraces while debugging some of the Join tests. You're absolutely right that a ScorerSupplier is created for each doc ID range -- not just the Scorer. Also, the test that I "helped" Vamsi with didn't really get fixed by moving construction of the TermsEnum into the get() method -- it just didn't happen to fail that time. *facepalm*
Please ignore my unfounded panic :D Thanks, Froh On Thu, Sep 12, 2024 at 1:33 AM Luca Cavanna <java...@apache.org> wrote: > Thanks for raising this Michael. > > I see a risk as well around removing the deprecated search(Query, > Collector) method by using a sequential collector manager. Previously, we > would happily execute sequentially despite an executor being provided to > the searcher (because search(Query, Collector) bypasses concurrency > entirely). After the migration, we would execute concurrently, hence the > collector manager would throw an exception as soon as more than one slice > gets searched. The sequential collector manager can be used in tests to > overcome the lack of a collector manager, but that only moves the problem > to the user side: what are users of these collectors going to do given they > need a collector manager to call search, and we don't provide one? Their > only option would be to not set an executor to their searcher. Maybe it is > a possibility to clearly document that but is it acceptable to require > users to have a non concurrent index searcher instance around to use for > non concurrent queries? There's only a few cases of this fortunately. > There's also a couple of cases like QueryUtils and JoinUtil where we have > utility methods that call search internally and accept an external > searcher. Those searchers may have an executor set to them, and the only > safe way to migrate these is to add concurrent collector managers support > or explicitly state that the provided search shouldn't have an executor set > to it. Another option if we are not happy with the workaround I mentioned, > is to consider leaving search(Query, Collector) deprecated in Lucene 10 and > removing it in Lucene 11. It is a shame because we are not far off, but I > am not sure that this warrants delaying the release. > I am not entirely sure how this aligns with the risk you mentioned, which > cases of SimpleCollector are you referring to specifically? > > Regarding your second concern around intra-segment concurrency: while I > had to adapt a couple of tests to be intra-segment ready as they made wrong > assumptions, we are now leveraging intra-segment concurrency in all tests > (provided that the searcher is created using LuceneTestCase#newSearcher), > besides when DrillSideways is used. I have seen a couple of post-merge > failures that may be related which I will look into, but nothing that would > suggest that the design is entirely problematic. When you retrieve a scorer > supplier or a bulk scorer you provide a LeafReaderContext. The overall > expectation is that you get a different instance each time, regardless of > whether you have already seen the segment or not. If that is the case there > is no state shared between threads, because ScorerSuppliers should not get > shared across threads? It is not the expectation that the support for > intra-segment concurrency requires bulk scorers and scorer suppliers to > become thread-safe. > With that in mind, I checked DefaultScorerSupplier and I do not see why it > would not work, as long as each call to retrieve a scorer supplier returns > a new instance of if that points to a new instance of Scorer, which holds > state created as part of that same scorerSupplier call. The problem that we > have is that we duplicate ahead of time work for partitions of the same > segment (tracked in https://github.com/apache/lucene/issues/13745), > because if we need to pull doc_values, we will do so for the same segment > multiple times. I would assume that if something is off with > DefaultScorerSupplier, tests would show that clearly, as it is widely used. > I also checked FeatureQuery, and I see that each call to > scorerSupplier(LeafReaderContext) returns a new instance of the supplier > which points to different TermsEnum instance retrieved multiple times for > the same segment. Removing this duplication will require additional work, > and there will be bugs, or incorrect assumptions made in existing scorer > supplier instances, but those should not be too hard to fix. > Does this make sense to you? Perhaps there are additional changes to make > in the migrate guide or javadocs to clarify what I described, let me know > what you think. > > Cheers > Luca > > On Thu, Sep 12, 2024 at 9:42 AM Michael Froh <msf...@gmail.com> wrote: > >> Oh, I forgot to mention: >> >> I think we should deprecate the DefaultScorerSupplier constructor that >> takes a Scorer. There's no way that works with intra-segment concurrency. >> >> Maybe we remove DefaultScorerSupplier altogether? >> >> On Thu, Sep 12, 2024 at 12:35 AM Michael Froh <msf...@gmail.com> wrote: >> >>> I'm a big fan of both of Luca's topics. I'd like to raise a small red >>> flag around them, though, since they seem to be connected. >>> >>> Working through the join module and helping my colleague @harshavamsi on >>> the QueryUtils side, I see two layers of unpreparatedness for the modern >>> "concurrency first" architecture. (Again, I want to make clear that I think >>> the modern architecture is the way to go and we can and should get there in >>> time for Lucene 10.) >>> >>> 1. There are several uses of SimpleCollector, where it's assumed that >>> one collector will collect all results on a single thread. With the >>> deprecated method, this forces single-threaded behavior all the time. In my >>> opinion, these represent 13+ year technical debt for cases where you >>> couldn't properly use an IndexSearcher to do concurrent searches. >>> 2. With the merge of intra-segment searches, we have another layer: >>> ScorerSuppliers that share mutable state across the Scorers that they >>> produce. For example, @harshavamsi came across a case today in the sigmoid >>> function for FeatureQuery where a TermsEnum was created in the >>> ScorerSupplier and passed into the Scorers. Each Scorer shared the same >>> TermsEnum. What changed? In the old concurrency model, one thread might >>> search a few segments, but each segment was guaranteed to only be searched >>> by one thread. Now, with intra-segment concurrency, we produce one >>> ScorerSupplier per segment, but may produce multiple Scorers across >>> different threads. If the ScorerSupplier produces some mutable object and >>> shares it across the resulting Scorers, you're going to have a bad time. >>> Fun fact: back in 2012, we had an office Halloween party and I dressed as >>> the thing that scares me the most. I printed a picture of Texas (since >>> everyone recognizes Texas) with a TV remote control mute button in the >>> middle. I sewed it to my shirt in the four corners. It was mutable state >>> held by multiple threads. >>> >>> I definitely think we should address these before the Lucene 10 release, >>> as they provide a clean break from the old world. I also think it's a >>> decent amount of work (but not unsurmountable). I'm also maybe no longer a >>> fan of the helper method that Greg added in his PR for the monitor module, >>> since it risks sweeping non-threadsafe code under the rug, if folks make >>> single-threaded tests (which is essentially what they've been doing all >>> along -- see my first point above). >>> >>> I haven't properly looked into the scope of my second point above, but >>> I've seen at least two cases in the past two days. Hopefully it's not too >>> bad, but it might be a risk. I think the first point is still pretty easy >>> to address. >>> >>> Thanks, >>> Froh >>> >>> On Thu, Aug 29, 2024 at 2:15 AM Luca Cavanna <l...@elastic.co.invalid> >>> wrote: >>> >>>> For Lucene 10.0, I have two topics to raise: >>>> >>>> 1. Remove the deprecated IndexSearcher#search(Query, Collector) in >>>> favour of IndexSearcher#search(Query, CollectorManager) ( >>>> https://github.com/apache/lucene/issues/12892): this involves removing >>>> the leftover usages in facet, grouping, join and test-framework, plus in >>>> some tests. A list of the leftover usages is in the description of the >>>> issue. It would be great to complete this for Lucene 10, otherwise this >>>> deprecated method and usages will stick around for much longer. What do >>>> others think? Should we make this a blocker for the release? I think this >>>> is not a huge effort and it is parallelizable across different people. >>>> >>>> 2. Intra-segment concurrency ( >>>> https://github.com/apache/lucene/pull/13542): current thinking is to >>>> add support for partitioning segments when searching, and searching across >>>> segment partitions concurrently. My intention is to introduce breaking >>>> changes and documentation in Lucene 10 (really only the basics), >>>> without switching the default slicing of IndexSearcher to create segment >>>> partitions. We will want to leverage segment partitions in testing. More >>>> iterations are going to be needed to remove duplicated work across >>>> partitions of the same segment, which is my next step, but currently out of >>>> scope for Lucene 10. Judging from the reviews I got so far, my PR is not >>>> far and I am working on it to address comments, polish it a bit more and >>>> merge it soon. >>>> >>>> Feedback is welcome >>>> >>>> Cheers >>>> Luca >>>> >>>> On Wed, Aug 28, 2024 at 3:05 PM Adrien Grand <jpou...@gmail.com> wrote: >>>> >>>>> Thanks Mike. >>>>> >>>>> On Wed, Aug 28, 2024 at 2:16 PM Michael McCandless < >>>>> luc...@mikemccandless.com> wrote: >>>>> >>>>>> I think maybe also https://github.com/apache/lucene/issues/13519 >>>>>> should be a blocker? It looks like 8 bit vector HNSW quantization is >>>>>> broken (unless I'm making a silly mistake with luceneutil tooling). >>>>>> >>>>>> I've also set its milestone to 10.0.0. >>>>>> >>>>>> Do we really not have a way to mark an issue a blocker for a given >>>>>> release? That's insane. OK well I went and created "blocker" label, and >>>>>> added that to GH 13519. Greg, I'll also go mark your linked issue as >>>>>> "blocker". >>>>>> >>>>>> Mike McCandless >>>>>> >>>>>> http://blog.mikemccandless.com >>>>>> >>>>>> >>>>>> On Sat, Aug 24, 2024 at 2:33 PM Uwe Schindler <u...@thetaphi.de> >>>>>> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> I updated Policeman Jenkins to have JDK 23 RC and JDK 24 EA releases. >>>>>>> >>>>>>> Uwe >>>>>>> >>>>>>> P.S.: Unfortunately I have to update the macOS Hackintosh VM to have >>>>>>> a newer operating system version: JDK 22 and later no longer run on this >>>>>>> machine. >>>>>>> Am 23.08.2024 um 10:41 schrieb Uwe Schindler: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> In 9.x there's still the backport of >>>>>>> https://github.com/apache/lucene/pull/13570 to be done. The PR >>>>>>> apperas in the changelog, but was not backported yet. Chris and I will >>>>>>> do >>>>>>> this soon. >>>>>>> >>>>>>> 9.last release on Sept 22 fits perfectly with the JDK 23 release >>>>>>> (and we will have Panama Vector Support). I am seeting up Jenkins Job >>>>>>> with >>>>>>> latest RC now to verify all vector stuff works with 23. >>>>>>> >>>>>>> Uwe >>>>>>> Am 08.08.2024 um 18:50 schrieb Adrien Grand: >>>>>>> >>>>>>> Hello everyone, >>>>>>> >>>>>>> As previously discussed >>>>>>> <https://lists.apache.org/thread/4bhnkkvvodxxgrpj4yqm5yrgj0ppc59r>, >>>>>>> I plan on releasing 9.last and 10.0 under the following timeline: >>>>>>> - ~September 15th: 10.0 feature freeze - main becomes 11.0 >>>>>>> - ~September 22nd: 9.last release, >>>>>>> - ~October 1st: 10.0 release. >>>>>>> >>>>>>> Unless someone shortly volunteers to do a 9.x release, this 9.last >>>>>>> release will likely be 9.12. >>>>>>> >>>>>>> As these dates are coming shortly, I would like to start tracking >>>>>>> blockers. Please reply to this thread with issues that you know about >>>>>>> that >>>>>>> should delay the 9.last or 10.0 releases. >>>>>>> >>>>>>> Chris, Uwe: I also wanted to check with you if this timeline works >>>>>>> well with regards to supporting Java 23 in 9.last and 10.0? >>>>>>> >>>>>>> -- >>>>>>> Adrien >>>>>>> >>>>>>> -- >>>>>>> Uwe SchindlerAchterdiek 19, D-28357 Bremen >>>>>>> <https://www.google.com/maps/search/Achterdiek+19,+D-28357+Bremen?entry=gmail&source=g>https://www.thetaphi.de >>>>>>> eMail: u...@thetaphi.de >>>>>>> >>>>>>> -- >>>>>>> Uwe SchindlerAchterdiek 19, D-28357 Bremen >>>>>>> <https://www.google.com/maps/search/Achterdiek+19,+D-28357+Bremen?entry=gmail&source=g>https://www.thetaphi.de >>>>>>> eMail: u...@thetaphi.de >>>>>>> >>>>>>> >>>>> >>>>> -- >>>>> Adrien >>>>> >>>>
