krickert commented on PR #1163: URL: https://github.com/apache/opennlp/pull/1163#issuecomment-4940102861
Thanks. Addressed in b76eef40, with two deliberate keeps: Main points: - SharingStemmer.stemAll: fixed, it forwards to the per-thread delegate; a test wraps a multi-output delegate and asserts the full result list survives. - State release: all three thread-routing stemmers (CachingStemmer, SharingStemmer, SnowballStemmer) expose clearThreadLocalState(), mirroring the ME components. On the owning thread the retained state object is reset (a test observes the cache emptying by counting delegate calls); on a worker thread the per-thread entry is removed and rebuilt on next use (tested on a spawned thread). The owner-reset lambdas are therefore reachable and tested. - Stemmer.stemAll kept: the review is right that no bundled multi-output stemmer exists yet, but the wrapper-forwarding bug this round fixed is exactly the class of defect that appears when the hook is retrofitted later, and a stacked branch already exercises stemAll against the new light stemmer tier. The javadoc now states the forwarding obligation for wrappers. - StemmerFactory name kept with the suggested disclaimer: the javadoc now states it is a plain supplier of configured stemmers and takes no part in the BaseToolFactory model-manifest mechanism. - Per-call routing cost for confined use: SnowballStemmerFactory.newStemmer() now returns a thread-confined product driving its engine through a plain field, no thread routing at all. That restores a zero-indirection path for the one-instance-per-thread pattern and removes the double indirection inside the caching and sharing wrappers, which build their per-thread delegates from the factory. - Virtual threads: the 34x claim is now qualified in the CachingStemmer javadoc and BENCHMARKS.md: the cache is keyed to the physical thread, pays off on reused platform threads, and a virtual-thread-per-task executor starts every task cold. Minor: - Double indirection through factory products: fixed by the confined product above. - StemmerFactory.stem(word) convenience: dropped. - TermAnalyzer.Builder.stem(StemmerFactory) name kept: the overload pair is documented, a null literal does not compile against either overload without a cast either way, and the factory overload adding caching is the documented reason the overload exists. The javadoc now spells out the thread-keyed cache semantics. - Null validation: IllegalArgumentException across the new surface; SnowballStemmerFactory no longer throws NPE. - Constructor guard: SnowballStemmer(ALGORITHM, int) validates algorithm and repeat like the factory, tested. - Repeat pass-through: tested with a witness word (internationalization stems to internation at repeat 1 and intern at repeat 2). - LRU initial capacity: sized for the load factor so a full cache never rehashes. - NormalizationProfilesTest style: inline qualified names and var replaced with imports and explicit types. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
