----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8033/#review13522 -----------------------------------------------------------
build-common.xml <https://reviews.apache.org/r/8033/#comment28998> I believe we want to add an empty target here, with a comment saying to override this target name in the subproject build.xml if that subproject needs to implement. Basically treat build-common.xml as an abstract class and this target as a no-op default implementation. build-common.xml <https://reviews.apache.org/r/8033/#comment28992> Please change this to: <echo message="${ant.project.name}"/> In the build, ant will print the target name by default, so this message does not provide any additional info. The info ant does not give you be default is which submodule is running the target, so printing the above message is quite useful when debugging. build-common.xml <https://reviews.apache.org/r/8033/#comment28997> Please remove this antcall, and instead we'll use target dependencies. build-common.xml <https://reviews.apache.org/r/8033/#comment28996> Can you remove "mvn-init" – I don't believe its needed anymore, as "mvn-dependencies" already depends on it. build.xml <https://reviews.apache.org/r/8033/#comment28990> We should remove this if webhcat is now packaged in a subdirectory. build.xml <https://reviews.apache.org/r/8033/#comment28991> We should remove this if webhcat-java-client is now packaged in a subdirectory. pom.xml <https://reviews.apache.org/r/8033/#comment28993> Please remove this line. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment28999> This can be removed by the refactor mentioned in a build-common.xml comment. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment28994> Please add as the first line in this target: <echo message="${ant.project.name}"/> This can be renamed "compile-resource" to override the target specified in build-common.xml. I don't remember for sure, but this might need to come before the import, but this will require testing. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment28995> Indentation one character to the right. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment29000> Since this happens after compiling sources this mkdir should not be necessary. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment29001> Should this be webhcat* now? webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment29002> This can be one line with: <fileset dir="${src.dir}" defaultexcludes="yes"/> Since there are no nested elements we can consolidate to a single statement. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment29003> Please remove this line. webhcat/svr/build.xml <https://reviews.apache.org/r/8033/#comment29004> Please remove this line. webhcat/svr/pom.xml <https://reviews.apache.org/r/8033/#comment29005> Please remove all the "trailing indentation" lines. webhcat/svr/pom.xml <https://reviews.apache.org/r/8033/#comment29008> Doesn't jerset-servlet pull in jersey-server and jersey-core? I believe we can remove these explicit deps. webhcat/svr/pom.xml <https://reviews.apache.org/r/8033/#comment29006> Is this needed? I believe it gets pulled in transitively from jackson-mapper-asl. webhcat/svr/pom.xml <https://reviews.apache.org/r/8033/#comment29007> Let's remove this as the section is empty. webhcat/svr/src/main/bin/webhcat_config.sh <https://reviews.apache.org/r/8033/#comment29009> Let's make this: PID_DIR=${WEBHCAT_PID_DIR:-.} PID_FILE=${PID_DIR}/webhcat.pid My guess is many users would want to put this in /var/run instead of the install dir. Putting the pid file inside the install dir is not really the standard way of doing things, so we should make that optional. webhcat/svr/src/main/bin/webhcat_config.sh <https://reviews.apache.org/r/8033/#comment29010> Let's ge the version number out of this file. It could be something like: WEBHCAT_JAR=$(ls webhcat-*.jar) webhcat/svr/src/main/bin/webhcat_config.sh <https://reviews.apache.org/r/8033/#comment29011> Why not reuse HCAT_HOME? Its basically the same thing, and simplifies the env variables that need set. Nit: please use: $(cmd to run) instead of: `cmd to run` I find the former less error-prone, easier to read, and can be nested. webhcat/svr/src/main/bin/webhcat_config.sh <https://reviews.apache.org/r/8033/#comment29012> Nit: please use "source" instead of "." webhcat/svr/src/main/config/webhcat-default.xml <https://reviews.apache.org/r/8033/#comment29013> Does this property still work? We don't bundle hive anymore (and the version number is incorrect). webhcat/svr/src/main/config/webhcat-default.xml <https://reviews.apache.org/r/8033/#comment29014> Does this still work? - Travis Crawford On Nov. 16, 2012, 3:08 a.m., Thejas Nair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8033/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2012, 3:08 a.m.) > > > Review request for hcatalog, Alan Gates and Travis Crawford. > > > Description > ------- > > first version of patch > > > This addresses bug hcatalog-549. > https://issues.apache.org/jira/browse/hcatalog-549 > > > Diffs > ----- > > ant/deploy.xml e0f5ea3 > build-common.xml 0e0af1c > build.xml 61ff391 > conf/templeton-default.xml bdd86d0 > conf/templeton-log4j.properties aa10199 > pom.xml 6ade4a6 > webhcat/svr/build.xml 070d9be > webhcat/svr/pom.xml 3ae03e7 > webhcat/svr/src/main/bin/templeton_config.sh fff987e > webhcat/svr/src/main/bin/templeton_server.sh c50c59c > webhcat/svr/src/main/bin/webhcat_config.sh PRE-CREATION > webhcat/svr/src/main/bin/webhcat_server.sh PRE-CREATION > webhcat/svr/src/main/config/webhcat-default.xml PRE-CREATION > webhcat/svr/src/main/config/webhcat-log4j.properties PRE-CREATION > webhcat/svr/src/main/java/org/apache/hcatalog/templeton/AppConfig.java > 28e96e3 > > Diff: https://reviews.apache.org/r/8033/diff/ > > > Testing > ------- > > > Thanks, > > Thejas Nair > >
