Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Bigtop, mail # dev - Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more dynamic


Copy link to this message
-
Re: Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more dynamic
Sean Mackrory 2013-07-15, 15:18
Eww - different version of RB than I'm used to. I thought those comments
were going to be attached as replies to yours. Each comment was a reply to
one of yours, however - and they should all be on the same line - let me
know if anything is not clear.

On Mon, Jul 15, 2013 at 8:13 AM, Sean Mackrory <[EMAIL PROTECTED]> wrote:

>    This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12526/
>    bigtop-packages/src/common/hadoop/install_hadoop.sh<https://reviews.apache.org/r/12526/diff/1/?file=321039#file321039line321> (Diff
> revision 1)
>
> 320
>
> chmod 644 $HTTPFS_ETC_DIR/conf.empty/tomcat-deployment/*
>
>   Good catch - does look like it should be. Will fix...
>
>
>    bigtop-packages/src/common/oozie/install_oozie.sh<https://reviews.apache.org/r/12526/diff/1/?file=321040#file321040line186> (Diff
> revision 1)
>
> 185
>
> cp ${BUILD_DIR}/oozie-server/conf/ssl/ssl-web.xml ${SERVER_LIB_DIR}/webapps-ssl/oozie/WEB-INF/web.xml
>
>   We shouldn't be changing it in both places, if that's what you're asking. Having a specific "feature" in the init script to deal with ssl-server.xml is something Bruno expressed concern about on Jira too, so yes I am considering other approaches to this part.
>
>
>    bigtop-packages/src/common/oozie/install_oozie.sh<https://reviews.apache.org/r/12526/diff/1/?file=321040#file321040line186> (Diff
> revision 1)
>
> 185
>
> cp ${BUILD_DIR}/oozie-server/conf/ssl/ssl-web.xml ${SERVER_LIB_DIR}/webapps-ssl/oozie/WEB-INF/web.xml
>
>   We shouldn't be changing it in both places, if that's what you're asking. Bruno also expressed concern about the specific "feature" in the init to enable SSL, however, so I am considering different approaches to this aspect if it.
>
> We should not to have webapps/ under /etc/conf because it can contain binary artifacts, but this is one case where some configuration under webapps has had to change.
>
>
>    bigtop-packages/src/common/oozie/install_oozie.sh<https://reviews.apache.org/r/12526/diff/1/?file=321040#file321040line191> (Diff
> revision 1)
>
> 190
>
> cp ${EXTRA_DIR}/catalina.properties ${CONF_DIR}/tomcat-deployment/conf/
>
>   We could. I don't see much of a difference. Do you?
>
>
>    bigtop-packages/src/common/oozie/oozie.init<https://reviews.apache.org/r/12526/diff/1/?file=321042#file321042line59> (Diff
> revision 1)
>
> 59
>
>     ln -s /usr/lib/oozie/webapps-ssl ${DEPLOYMENT_TARGET}/webapps
>
>   What effect do you think forcefully removing it will have? It's already wrapped in a conditional to see if it exists. -f is more succinct, but am I missing something in your suggestion?
>
>
>    bigtop-packages/src/common/oozie/oozie.init<https://reviews.apache.org/r/12526/diff/1/?file=321042#file321042line59> (Diff
> revision 1)
>
> 59
>
>     ln -s /usr/lib/oozie/webapps-ssl ${DEPLOYMENT_TARGET}/webapps
>
>   What do you think forcefully removing it is going to do? It's more succinct than the existing conditional, but AFAIK there's no functional difference.
>
>
>    bigtop-packages/src/common/oozie/oozie.init<https://reviews.apache.org/r/12526/diff/1/?file=321042#file321042line60> (Diff
> revision 1)
>
> 60
>
>     cp ${DEPLOYMENT_TARGET}/conf/ssl/ssl-server.xml ${DEPLOYMENT_TARGET}/conf/server.xml
>
>   They can still use their own custom configuration, but if they're upgrading from a previous version they'll need to copy in tomcat-deployment from conf.dist, so we ought to document that somewhere. (Is there a concept of "Release Notes" in Apache releases?)
>
> Not ideal, but all of the alternatives I can think of are much worse. We shouldn't be copying it into their custom config for them, we shouldn't leave it the way it is, etc...
>
>
>    bigtop-packages/src/common/sqoop/sqoop-server.sh<https://reviews.apache.org/r/12526/diff/1/?file=321046#file321046line27> (Diff
> revision 1)
>
> 27
>
>   ln -s ${SQOOP_HOME}/bin /${DEPLOYMENT_TARGET}/
>
>   Fixed. Should have no effect, though.
>
>
> - Sean Mackrory
>
> On July 12th, 2013, 10:16 p.m. UTC, Sean Mackrory wrote: