[
https://issues.apache.org/jira/browse/SOLR-5746?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Hoss Man updated SOLR-5746:
---------------------------
Attachment: SOLR-5746.patch
bq. Since I stored "raw" config values, I used SolrParam to do the type
conversion, but I didn’t find any API for a parameter removal. That’s why I’m
keeping the original NamedList, so that I can remove correctly read values and
keep track of the unknown ones.
my previous suggestion about using SolrParams was vague and misguided. i
_think_ i had in mind the idea of using SolrParams instead of the
{{Map<String,Object> propMap}} -- your latest patch that eliminates the
SolrParams and just uses the NamedList is definitely a better call.
bq. ... However, I didn't realise that solr.xml files are versioned the same
way as schema.xml files are. Should I bump the schema version to 1.6?
It's not explicitly versioned -- just hueristicly versioned based on wether
ConfigSolr detects by introspection that it's the "old style" or the "new
style" ... i think Jack may have just been confused about what this issue was
when he posted his comment.
----
I'm attaching some updates to your patch...
* skimming your changes made me realize there's a lot of cruft in this code
related to defered sys prop substitution that's no longer needed at all - so I
ripped that out.
* I'm not really a fan of the way you added "excludedElements" to the DOMUtils
method -- particularly since it still required the
{{namedList.removeAll(null);}} call which seemed sloppy. I'd much rather have
a tighter XPath that is very explicit about what we want out of the dom and
handle the excusions that way ... so i changed that.
* i added some explicit testing of {{<null name="..."/>}} since i wasn't
completley convinced your new code that that would work correctly.
* I think i misslead you a bit when i said we should validate configs being
declared multiple times -- it's not a good idea to up front check that nothing
is declared more then once, beause a week from now someone may in fact want to
add something to solr.xml that can be specified multiple times. The better
place for this type of validation is in storeConfigProperty, because at that
point we know we expect there to be a single value.
** this does unfortunately mean it aborts early the first time it finds a
duplicated key, so some of your tests had to be changed.
* I switched the check for unknown options to be per section so the error msgs
could include the section details as well.
* String.format must be used with Locale.ROOT to prevent locale sensitive
behavior.
** {{ant check-forbidden-apis}} will point out stuff like this for you in the
lucene/solr code base
* relaxed the int parsing so that small {{<long>}} values are fine, but large
longs still throw an error
** added test for both cases
* added some checks that the sections themselves weren't being duplicated (ie:
if a user adds a <solrcloud> section totheir solr.xml, we want to give them an
error if another <solrcloud> section already existed higher up in the file)
* some general test refactoring...
** no need to construct new Random instances -- just use random()
** eliminated a lot of unneccessary file creation in the tests by using
{{ConfigSolr.fromString(loader, solrXml);}} instead of
{{FileUtils.writeFile(...)}} and {{ConfigSolr.fromSolrHome(...)}}
** switched to the lucene convention of testmethod naming to eliminate ~20
lines of {{@Test}} annotations (the verbosity is why our test runner explicitly
lets us continue to use the JUnit3 convention)
----
I think this is probably ready to go -- but it would be nice to get some review
from [~romseygeek] and/or [~erickerickson] since they know this code the best
... and of course, [~maciej.zasada]: you've clearly been looking at this code a
lot the last few days, do you ou have any additional thoughts on my revised
patch?
> solr.xml parsing of "str" vs "int" vs "bool" is brittle; fails silently;
> expects odd type for "shareSchema"
> --------------------------------------------------------------------------------------------------------------
>
> Key: SOLR-5746
> URL: https://issues.apache.org/jira/browse/SOLR-5746
> Project: Solr
> Issue Type: Bug
> Affects Versions: 4.3, 4.4, 4.5, 4.6
> Reporter: Hoss Man
> Attachments: SOLR-5746.patch, SOLR-5746.patch, SOLR-5746.patch,
> SOLR-5746.patch, SOLR-5746.patch
>
>
> A comment in the ref guide got me looking at ConfigSolrXml.java and noticing
> that the parsing of solr.xml options here is very brittle and confusing. In
> particular:
> * if a boolean option "foo" is expected along the lines of {{<bool
> name="foo">true</bool>}} it will silently ignore {{<str
> name="foo">true</str>}}
> * likewise for an int option {{<int name="bar">32</int>}} vs {{<str
> name="bar">32</str>}}
> ... this is inconsistent with the way solrconfig.xml is parsed. In
> solrconfig.xml, the xml nodes are parsed into a NamedList, and the above
> options will work in either form, but an invalid value such as {{<bool
> name="foo">NOT A BOOLEAN</bool>}} will generate an error earlier (when
> parsing config) then {{<str name="foo">NOT A BOOLEAN</str>}} (attempt to
> parse the string as a bool the first time the config value is needed)
> In addition, i notice this really confusing line...
> {code}
> propMap.put(CfgProp.SOLR_SHARESCHEMA,
> doSub("solr/str[@name='shareSchema']"));
> {code}
> "shareSchema" is used internally as a boolean option, but as written the
> parsing code will ignore it unless the user explicitly configures it as a
> {{<str/>}}
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
