[
https://issues.apache.org/jira/browse/SOLR-11687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16268093#comment-16268093
]
Erick Erickson edited comment on SOLR-11687 at 11/28/17 4:23 AM:
-----------------------------------------------------------------
I've worked up the skeleton of a patch I'll look over and attach tomorrow after
running tests. I have a couple of nocommits I'd like to draw attention to, in
particular what do we do if, say, the index.properties file has
{{index=}}
or, more generically if {{Properties.getProperty("index")}} returns null or an
empty string? It seems like returning the default {{dataDir/index/}} is correct.
The simple notion of returning null looks ugly, there are numerous places that
pretty much assume the response is _not_ null and some of them are convoluted.
Catching them all would be a pain and I'm not sure would add any value. Failing
to catch them would result in NPEs, just kicking the can down the road.
Throwing a RuntimeException fits the bill of "something a programmer can't
reasonably be expected to handle". If we throw a checked exception it looks at
first glance like there are a lot of places the code needs to be touched. Ditto
I'm not sure any value would be added by using a checked exception
Anyway, I'll see if tests run tonight and attach the file if it looks OK in the
morning.
was (Author: erickerickson):
I've worked up the skeleton of a patch I'll look over and attach tomorrow after
running tests. I have a couple of nocommits I'd like to draw attention to, in
particular what do we do if, say, the index.properties file has
{{index=}}
or, more generically if {{ Properties.getProperty("index") }} returns null or
an empty string? It seems like returning the default {{ dataDir/index/ }} is
correct.
The simple notion of returning null looks ugly, there are numerous places that
pretty much assume the response is _not_ null and some of them are convoluted.
Catching them all would be a pain and I'm not sure would add any value. Failing
to catch them would result in NPEs, just kicking the can down the road.
Throwing a RuntimeException fits the bill of "something a programmer can't
reasonably be expected to handle". If we throw a checked exception it looks at
first glance like there are a lot of places the code needs to be touched. Ditto
I'm not sure any value would be added by using a checked exception
Anyway, I'll see if tests run tonight and attach the file if it looks OK in the
morning.
> SolrCore.getNewIndexDir falsely returns {dataDir}/index on any IOException
> reading index.properties
> ---------------------------------------------------------------------------------------------------
>
> Key: SOLR-11687
> URL: https://issues.apache.org/jira/browse/SOLR-11687
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Erick Erickson
> Assignee: Erick Erickson
>
> I'll link the originating Solr JIRA in a minute (many thanks Nikolay).
> right at the top of this method we have this:
> {code}
> String result = dataDir + "index/";
> {code}
> If, for any reason, the method doesn't complete properly, the "result" is
> still returned. Now for instance, down in SolrCore.cleanupOldIndexDirectories
> the "old" directory is dataDir/index which may point to the current index.
> This seems particularly dangerous:
> {code}
> try {
> p.load(new InputStreamReader(is, StandardCharsets.UTF_8));
> String s = p.getProperty("index");
> if (s != null && s.trim().length() > 0) {
> result = dataDir + s;
> }
> } catch (Exception e) {
> log.error("Unable to load " + IndexFetcher.INDEX_PROPERTIES, e);
> } finally {
> IOUtils.closeQuietly(is);
> }
> {code}
> Should "p.load" fail for any reason whatsoever, we'll still return
> dataDir/index.
> Anyone want to chime on on what the expectations are here before I dive in?
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
