FYI I have little availability this week so Luca and Chris offered to help with the release. Thank you both!
Le sam. 14 sept. 2024, 18:07, Luca Cavanna <java...@apache.org> a écrit : > On the topic of removing search(Query, Collector), I think it is unlikely > that we will be able to complete that task before the Lucene 10 branch is > cut. It would be nice to still work on the remaining issues, but it's > probably good to have less urgency around it hence more time to think about > how to properly address the remaining issues. I think that removing a > deprecated method leaving users without a proper replacement is not a good > trade-off. > > I have an outstanding PR around CollectorManager#forSequentialExecution, > based on the risk I described in my previous message.I am now concluding > that it would be safer to remove such a public method from branch_9x before > it gets released. I described the reason in the PR I just opened: > https://github.com/apache/lucene/pull/13790 . I'd like to know what folks > think about this. I myself have changed my mind a few times on the topic. > > On Thu, Sep 12, 2024 at 10:09 PM Michael Froh <msf...@gmail.com> wrote: > >> 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 >>>>>>> >>>>>>
