David Smiley

+1 thanks Kevin

Should this be catching RuntimeException as well so we can capture these too? For example processErrorsAndResponse can throw RemoteSolrException (which subclasses RuntimeException).

If only we renamed "Highlighter" to "OriginalHighlighter", maybe folks wouldn't continue to using this thing. Is the UnifiedHighlighter not satisfying you, and if so, why not?

This is nifty! I wonder if it'd be worthwhile for Lucene itself to track this small bit of metadata so that it's persistent?

The former is more intuitive to me -- how much more data do we write beyond the initial segment flush. This is the added cost of a system immutable files...

This PR does not use `String.intern` which was the previous concern. So what's wrong here?

The map isn't static. Even if there was a static map, *if* it was expressly used for known static strings, then it wouldn't be a leak but just re-use of...

Could you comment on the pertinent line of code please? `canonicalStrings` is not static. Maybe you are thinking of the ThreadLocal? That's some temporary byte buffer.

The ThreadLocal aspect was a needless addition here; it could be reverted. > the private map just leaks for the caller by unnecessarily saving strings. it's a lot of overhead...

There is somewhat a matter of taste at stake, so I can appreciate Robert's point of view that DataInput is nice the way it is without adding this to it...