dsmiley commented on code in PR #962:
URL: https://github.com/apache/solr/pull/962#discussion_r957814422
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -219,8 +218,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
if (zkClient.exists("/solr.xml", true)) {
log.info("solr.xml found in ZooKeeper. Loading...");
byte[] data = zkClient.getData("/solr.xml", null, null, true);
- return SolrXmlConfig.fromInputStream(
Review Comment:
This change does not use "data" that was just fetched; surely it can't be
correct?
##########
solr/test-framework/src/java/org/apache/solr/util/TestHarness.java:
##########
@@ -167,7 +167,7 @@ public TestHarness(
* @param solrXml the text of a solrxml
*/
public TestHarness(Path solrHome, String solrXml) {
- this(SolrXmlConfig.fromString(solrHome, solrXml));
Review Comment:
can't be right; solrXml is ignored
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -230,12 +228,8 @@ public static NodeConfig fromInputStream(
substituteProps = new Properties();
}
try {
- byte[] buf = IOUtils.toByteArray(is);
- try (ByteArrayInputStream dup = new ByteArrayInputStream(buf)) {
- XmlConfigFile config =
- new XmlConfigFile(loader, null, new InputSource(dup), null,
substituteProps);
- return fromConfig(solrHome, config, fromZookeeper);
- }
+ XmlConfigFile config = new XmlConfigFile(loader, null, null,
substituteProps);
Review Comment:
> how is the InputStream is used here?
Apparently, @heythem changed this method to not even take an InputStream.
But I don't see how it can work with those two nulls; how would the
XmlConfigFile even know what to load?
##########
solr/core/src/test/org/apache/solr/metrics/reporters/SolrSlf4jReporterTest.java:
##########
@@ -51,7 +51,7 @@ public void testReporter() throws Exception {
String solrXml =
FileUtils.readFileToString(
Paths.get(home.toString(), "solr-slf4jreporter.xml").toFile(),
"UTF-8");
- NodeConfig cfg = SolrXmlConfig.fromString(home, solrXml);
Review Comment:
can't be right; solrXml is ignored
##########
solr/core/src/test/org/apache/solr/metrics/reporters/SolrGraphiteReporterTest.java:
##########
@@ -58,7 +58,7 @@ public void testReporter() throws Exception {
String solrXml =
FileUtils.readFileToString(
Paths.get(home.toString(),
"solr-graphitereporter.xml").toFile(), "UTF-8");
- NodeConfig cfg = SolrXmlConfig.fromString(home, solrXml);
Review Comment:
can't be right; solrXml is ignored
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -413,7 +413,7 @@ private void readXml(
Map<String, IndexSchemaFactory.VersionedConfig> configCache,
ResourceProvider rp)
throws IOException {
- XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/",
null);
+ XmlConfigFile xml = new XmlConfigFile(loader, name, "/config/", null);
Review Comment:
I confirm this by code inspection. From what I see, ResourceProvider should
perhaps be nothing more than a POJO; it doesn't even _need_ methods.
CC @noblepaul -- you added a constructor to XmlConfigFile last October to
take a `Function<String, InputStream> fileSupplier` that seems could simply be
the InputStream directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
