SOLR-17864: Provide backwards support for "<str name="hideStackTrace">${solr.hideStackTrace:false}</str>" - #4893
SOLR-17864: Provide backwards support for "<str name="hideStackTrace">${solr.hideStackTrace:false}</str>"#4893epugh wants to merge 4 commits into
Conversation
…StackTrace:false}</str>" These xml subtitutions don't work like the normal deprecations do. Need to rethink a bit the logic for making it work in places like solr.xml and solrconfig.xml.
github-actions
Bot
added
documentation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Definitely need this for 10.1.
| * Maps a legacy/deprecated sys prop key (dot-separated form) to the current property it was | ||
| * replaced by, and whether the replacement is boolean-inverted relative to the legacy property. | ||
| */ | ||
| private record DeprecatedMapping(String currentName, boolean inverted) {} |
There was a problem hiding this comment.
okay/fine but merely for a boolean you could simply have a parallel set of inverted names. That's what I'd do.
|
Okay, having second thoughts on deprecation.... On a fresh bin/solr start -e techproducts, there are 2 deprecation warnings, both from the new fallback mechanism we just added: Worth flagging though: these two aren't a user leaving a stale config behind. They fire because the shipped solr.xml/code intentionally keeps historical token names permanently (host, zkClientTimeout — same category as hideStackTrace, which we deliberately decided not to rename in config), and bin/solr itself always sets -Dsolr.host.advertise=... at startup. So EnvUtils's fallback resolves these on every single default Solr startup, for every user, forever — not just for someone with an outdated custom config. That means this warning will show up in every fresh install's log from day one, which might read as alarming/noisy for something that's actually working exactly as designed. Unless we actually change the defaults that we ship to the new values....? |
| if (alreadyLogged.containsKey(featureId)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This first part is needless; surely putIfAbsent will handle this, right?
Is this an optimization? If it is, a comment should clearly say so. But I'm suspicious it's any faster than putIfAbsent; surely that one can bail fast. I looked at the code for it in ConcurrentHashMap and I think it's good.
Why did we not thoroughly change then; are we not sure we wanted to actually change? |
2 participants
https://issues.apache.org/jira/browse/SOLR-17864
Description
I thought migrating solr.hideStackTrace to solr.responses.stacktrace.enabled would be easy... it had been partly done. Then I discovered that in solrconfig.xml and solr.xml, the property substitution didn't leverage the deprecation mechanism.
So ${solr.hideStackTrace:false} wouldn't actually pick up a solr.responses.stacktrace.enabled value set on the command line — if you'd upgraded and only set the new property name, this token silently kept using its own inline default instead.
This was potentially a big problem for anyone migrating from an older version to a newer one who uses this feature.
The fix is in EnvUtils, not in the token. I initially assumed the fix meant rewriting the token to reference the new name — ${solr.responses.stacktrace.enabled:true} — but that's actually wrong and introduces a worse bug: plain ${...} substitution can't invert a value, and solr.responses.stacktrace.enabled is inverted relative to hideStackTrace (true=show vs. true=hide). Written that way, the token hides stack traces by default, and setting -Dsolr.responses.stacktrace.enabled=true (wanting them shown) actually hides them — the opposite of what's asked.
The correct fix: leave the token exactly as it was — ${solr.hideStackTrace:false} — and teach EnvUtils to resolve a legacy property name by falling back to its replacement's value (inverting it when the mapping is inverted) whenever the legacy name itself isn't set. That way solr.xml/solrconfig.xml never need to change at all; setting -Dsolr.responses.stacktrace.enabled= on the command line is enough, and it's correctly inverted under the hood.
Solution
Rethink a bit how EnvUtils work... Add a Record for tracking properties and their inverted status. Now wires in so older solr.xml etc can participate in reading the deprecated values.
Tests
add tests, and then did this script: