Hi Adrien, Thank you for your reply. At your suggestion I set out to put together a Lucene only test and realised what was actually going on...
PR#12380 introduced a _new_ method called `void finish() throws IOException` which clashed with Solr's existing finish method on the DelegatingCollector (extending the SimpleCollector) [0]. this introduced the second 'finish' call, one on Lucene side for the leaf collectors and one on Solr side for completing the DelegatingCollector. the new method fit so well with the existing method that I did not even see it clashing. Renaming the existing method solves all of this weirdness. Apologies for the noise, I was deep down the rabbit hole. thanks, alex [0] https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/DelegatingCollector.java#L83 On Thu, Sep 28, 2023 at 1:23 PM Adrien Grand <jpou...@gmail.com> wrote: > Hi Alex, > > I believe that your analysis is correct. > > > is it expected that the 'finish' method is idempotent? > > I don't expect `finish()` to be idempotent. It should not get called > multiple times per segment either, only once and when collection runs > successfully. Do you have a Lucene test case that reproduces this double > calling of finish()? > > I'm sorry this change broke Solr. I remember that Solr had post-collection > hooks, which felt like another case for adding this new API, but I > overlooked that it could break Solr by introducing a clash given that Solr > uses SimpleCollector. > > Maybe we should think of deprecating SimpleCollector in Lucene and > recommend going with Collector directly. SimpleCollector is mostly a > backward compatibility layer with the old (9 years old > <https://github.com/apache/lucene/issues/6590>) collector API, we already > moved some of Lucene's main collectors to the new API, e.g. > TopScoreDocCollector and TopFieldCollector. Let's move other collectors > too, e.g. FacetsCollector and friends? > > > > On Thu, Sep 28, 2023 at 12:31 AM Alex Deparvu <stilla...@apache.org> > wrote: > >> Hi, >> >> I am working on getting Solr upgraded to Lucene 9.8 [0] and I wanted to >> raise visibility on an issue I ran into. >> >> I believe PR#12380 [1] introduced a change that calls `finish()` on the >> LeafCollector [2]. The trouble is on Solr side we have a few collectors >> that extend SimpleCollector. (to be more precise there is a >> DelegatingCollector in between but that does not change things). >> SimpleCollector returns `this` on the `getLeafCollector` call, so now >> there are 2 calls to the `finish()` method on the same collector instance >> (one as a leaf, one at the end). >> >> One example I am working with is >> CollapsingQParserPlugin$OrdScoreCollector [3] where I am seeing a few tests >> fail because calling `finish` twice will mess up the results. I don't know >> yet if there are others. >> >> My first question is to validate this analysis with someone that knows >> this code (and perhaps the Solr code too), and ideally also take a quick >> look at my fix [4]. >> >> Second is related to PR#12380. is it expected that the 'finish' method is >> idempotent? per my tests this seems to be called twice now in some cases >> and it will be the case for any implementation extending SimpleCollector. >> also given that SimpleCollector's getLeafCollector method is final, there >> is almost no room for passing some state wrt. this being a leaf vs not. >> >> >> thanks, >> alex >> >> >> [0] https://github.com/apache/solr/pull/1958 >> [1] https://github.com/apache/lucene/pull/12380 >> [2] >> https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L779 >> [3] >> https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java#L594 >> [4] >> https://github.com/apache/solr/commit/fc8a2ffe8951f31aa3b65fac2adc9eaa3fee6258 >> >> > > -- > Adrien >
