I think it's easier to make the case that CachingTokenFilter should have been propagating reset() from it's fillCache() all along, and thus you would then use CachingTokenFilter in a more normal way – wrap it and call reset() then increment in a loop, etc., instead of knowing you need to reset() on what it wraps but not this token filter itself. That's weird. It's ab-normal for a TokenFilter to never propagate reset, so every user of CachingTokenFilter to date has worked around this by calling reset() on the underlying input instead of the final wrapping token filter (CachingTokenFilter in this case). To be clear, CachingTokenFilter.reset() didn't and still doesn't with this patch propagate reset(), it happens the one time it consumes the stream indirectly via incrementToken().
I mostly agree with this. Except i think this filter should propogate reset() from its own reset(), only once (then once cached, it obviously doesnt propogate any such call).
But then why propose an option? I would rather fix the behavior of the this thing and not have "two ways to do it". Consumers of the TS api already have a hard enough time.
We can do a break, since its 5.0, and there arent many consumers of cachingtokenfilter. But one of them is a big one, queryparser, which is not good. Another option is to deprecate it and just make a new one beside it with a different name and the new "good" reset behavior.