----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22848/#review46810 -----------------------------------------------------------
Great work Abe! I know this wasn't easy to figure out and get working. I did an initial review and made some comments. I haven't really looked at any of the tests yet. Also, do we need so many extra txt files for the tests? client/src/main/resources/search-batch-indexer-action-0.1.xsd <https://reviews.apache.org/r/22848/#comment82378> Shouldn't the minOccurs be "1" on these? I'm not sure, but I believe the xs:choice will enforce that only one of them is actually provided, right? core/pom.xml <https://reviews.apache.org/r/22848/#comment82369> This should stay at "provided" Also, can you put back the order? You swapped this and oozie-hadoop-test, so it's not a "real" change. core/src/main/java/org/apache/oozie/action/hadoop/SearchBatchIndexerActionExecutor.java <https://reviews.apache.org/r/22848/#comment82370> Can you explicitly list the imports instead of using * core/src/main/java/org/apache/oozie/action/hadoop/SearchBatchIndexerActionExecutor.java <https://reviews.apache.org/r/22848/#comment82371> It might be a good idea to add a config property to oozie-default to override this check just in case. Otherwise, I could see someone using a custom version of Hadoop where this could work (or at least they think it should) and we have no way of going around this check if it doesn't agree. core/src/main/java/org/apache/oozie/action/hadoop/SearchBatchIndexerActionExecutor.java <https://reviews.apache.org/r/22848/#comment82372> Should this be the following? conf = super.setupLauncherConf(conf, actionXml, appPath, context); docs/src/site/twiki/DG_SearchBatchIndexerActionExtension.twiki <https://reviews.apache.org/r/22848/#comment82374> It may make sense to add an "About" section (or some other name) saying that this refers to Cloudea Search, an Apache licensed Search thing built on Lucene and Solr that can be found at GITHUB_PAGE_OR_WEBPAGE. This is less well-known than the other actions types and isn't an Apache-run project. We can then also put the Hadoop 2 requirement in the same section. docs/src/site/twiki/DG_SearchBatchIndexerActionExtension.twiki <https://reviews.apache.org/r/22848/#comment82375> Can we call this something else? It's confusing with NAME-NODE. And also the "ok to" and "error to" transitions should have some other name, or it looks like a loop (e.g. SOMENAME2) docs/src/site/twiki/DG_SearchBatchIndexerActionExtension.twiki <https://reviews.apache.org/r/22848/#comment82376> You're only allowed to have one of these, right? docs/src/site/twiki/DG_SearchBatchIndexerActionExtension.twiki <https://reviews.apache.org/r/22848/#comment82377> If these additional arguments are required for the zookeeper and solrconfigs, why not make them required by the schema? I'm not sure, but I think this would do it; if not, it would be something similar: <xs:choice minOccurs="1" maxOccurs="1"> <xs:sequence> <xs:element name="zookeeper" type="xs:string" minOccurs="1" maxOccurs="1"/> <xs:element name="collection" type="xs:string" minOccurs="1" maxOccurs="1"/> </xs:sequence> <xs:sequence> <xs:element name="solrconfig" type="xs:string" minOccurs="1" maxOccurs="1"/> <xs:element name="shards" type="xs:string" minOccurs="1" maxOccurs="1"/> </xs:sequence> </xs:choice> sharelib/search/src/test/java/org/apache/oozie/action/hadoop/TestSearchBatchIndexerActionExecutor.java <https://reviews.apache.org/r/22848/#comment82379> Replace with explicit classes sharelib/search/src/test/resources/solr/conf/currency.xml <https://reviews.apache.org/r/22848/#comment82380> Please remove the trailing whitespace if possible. sharelib/search/src/test/resources/solr/conf/elevate.xml <https://reviews.apache.org/r/22848/#comment82381> Please remove the trailing whitespace if possible. sharelib/search/src/test/resources/solr/conf/lang/contractions_it.txt <https://reviews.apache.org/r/22848/#comment82383> Trailing whitespace sharelib/search/src/test/resources/solr/conf/lang/stoptags_ja.txt <https://reviews.apache.org/r/22848/#comment82384> Trailing whitespace sharelib/search/src/test/resources/solr/conf/schema.xml <https://reviews.apache.org/r/22848/#comment82382> Trailing whitespace throughout this file again if possible. - Robert Kanter On June 21, 2014, 12:11 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22848/ > ----------------------------------------------------------- > > (Updated June 21, 2014, 12:11 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1895 > https://issues.apache.org/jira/browse/OOZIE-1895 > > > Repository: oozie-git > > > Description > ------- > > - Provide 2 different paths of execution: Zookeeper config and solrconfig. > With solrconfig, --shards argument should be passed via "argument" in the > action xml. With the zookeeper config, --collection should be provided via > "argument" in the action xml. > - Go live mode not supported in secure clusters. > - Only available with Hadoop 2. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/cli/OozieCLI.java 33935d3 > client/src/main/resources/search-batch-indexer-action-0.1.xsd PRE-CREATION > core/pom.xml e152266 > > core/src/main/java/org/apache/oozie/action/hadoop/SearchBatchIndexerActionExecutor.java > PRE-CREATION > docs/src/site/twiki/DG_SearchBatchIndexerActionExtension.twiki PRE-CREATION > docs/src/site/twiki/index.twiki f078bf5 > pom.xml bad1e0f > sharelib/pom.xml df20294 > sharelib/search/pom.xml PRE-CREATION > > sharelib/search/src/main/java/org/apache/oozie/action/hadoop/SearchBatchIndexerMain.java > PRE-CREATION > > sharelib/search/src/test/java/org/apache/oozie/action/hadoop/TestSearchBatchIndexerActionExecutor.java > PRE-CREATION > sharelib/search/src/test/resources/morphlines/basic.conf PRE-CREATION > sharelib/search/src/test/resources/solr/conf/currency.xml PRE-CREATION > sharelib/search/src/test/resources/solr/conf/elevate.xml PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/contractions_ca.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/contractions_fr.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/contractions_ga.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/contractions_it.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/hyphenations_ga.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stemdict_nl.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stoptags_ja.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ar.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_bg.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ca.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_cz.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_da.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_de.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_el.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_en.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_es.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_eu.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_fa.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_fi.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_fr.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ga.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_gl.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_hi.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_hu.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_hy.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_id.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_it.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ja.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_lv.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_nl.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_no.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_pt.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ro.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_ru.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_sv.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_th.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/stopwords_tr.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/lang/userdict_ja.txt > PRE-CREATION > sharelib/search/src/test/resources/solr/conf/protwords.txt PRE-CREATION > sharelib/search/src/test/resources/solr/conf/schema.xml PRE-CREATION > sharelib/search/src/test/resources/solr/conf/solrconfig.xml PRE-CREATION > sharelib/search/src/test/resources/solr/conf/stopwords.txt PRE-CREATION > sharelib/search/src/test/resources/solr/conf/synonyms.txt PRE-CREATION > src/main/assemblies/sharelib.xml 891d9dc > webapp/pom.xml 93cfcef > > Diff: https://reviews.apache.org/r/22848/diff/ > > > Testing > ------- > > Wrote a couple of tests and manually tested on Kerberized environment. > > > Thanks, > > Abraham Elmahrek > >
