-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First cut at upgrading to Solr 6.6.2, Hadoop 2.6.0 and Cascading 2.7.1. #13
base: master
Are you sure you want to change the base?
Conversation
Hi @vmagotra - yes, I don't think we need the solr.xml file any longer - just a conf dir. |
File coreProps = new File(coreDir, "core.properties"); | ||
|
||
// TODO copy the conf files over to the core dir | ||
File srcConfDir = new File(solrCoreDir, "conf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want the caller to provide the path to the conf dir, not the parent of the conf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree. I've pushed changes with that in effect.
Before we merge these changes into master, we'd probably want to also update the README.md file to explain this change.
@@ -32,8 +31,8 @@ | |||
|
|||
public abstract class AbstractSolrSchemeTest extends Assert { | |||
|
|||
private static final String SOLR_HOME_DIR = "src/test/resources/solr-home-4.1/"; | |||
protected static final String SOLR_CORE_DIR = SOLR_HOME_DIR + "collection1"; | |||
private static final String SOLR_HOME_DIR = "src/test/resources/solr-home-6.6.2/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need a SOLR_HOME_DIR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used on for the test when setting up the embedded solr server to query the index to verify that it was set up correctly.
@@ -0,0 +1,67 @@ | |||
<?xml version="1.0" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this file? It's for query time only, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't need it. I have cleaned up the test resources to get rid of all extraneous files.
Unless we're actually testing all of the Solr features, I'd get rid of the solr.xml, stopwords files, etc. that are in the |
coreDir.mkdirs(); | ||
File coreProps = new File(coreDir, "core.properties"); | ||
|
||
// TODO copy the conf files over to the core dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still a TODO
to copy over the files? Looks that that's being done below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up in latest push.
…rectory. Removed extraneous Solr conf files from the test resources.
@kkrugler - Please review the changes made after your review. |
@kkrugler - in order to maintain semantics with the previous version, I've kept the creation of the Solr Scheme to use a Solr core directory. However, in the Solr 6.x world I think the scheme should take just the Solr conf directory (since the layout of the directory in Solr 6 can have cores that only include the core.properties file and the conf may live under the configsets subdirectory).