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: > Review request for bigtop and Mark Grover. > By Sean Mackrory. > > *Updated July 12, 2013, 10:16 p.m.* > *Bugs: * BIGTOP-939 <https://issues.apache.org/jira/browse/BIGTOP-939> > *Repository: * bigtop > Description > > Explained on JIRA - uploaded here only for commenting convenience. > > Testing > > Explained on JIRA - uploaded here only for commenting convenience. > > Diffs > > - bigtop-packages/src/common/hadoop/hadoop-httpfs.svc (de4d6d2) > - bigtop-packages/src/common/hadoop/httpfs.default (e48e608) > - bigtop-packages/src/common/hadoop/install_hadoop.sh (ed9cb5c) > - bigtop-packages/src/common/oozie/install_oozie.sh (c27dd64) > - bigtop-packages/src/common/oozie/oozie-env.sh (dd051f7) > - bigtop-packages/src/common/oozie/oozie.init (4600841) > - bigtop-packages/src/common/solr/install_solr.sh (4e09e40) > - bigtop-packages/src/common/solr/solr-server.init.debian (5b8b862) > - bigtop-packages/src/common/sqoop/install_sqoop.sh (df4f489) > - bigtop-packages/src/common/sqoop/sqoop-server.sh (b1de301) > - bigtop-packages/src/common/sqoop/sqoop.default (cdec81c) > - bigtop-packages/src/common/sqoop/sqoop.sh (afe1ac6) > - bigtop-packages/src/deb/hadoop/hadoop-httpfs.install (fe1a462) > - bigtop-packages/src/deb/sqoop/sqoop.install (0386c7a) > - bigtop-packages/src/rpm/hadoop/SPECS/hadoop.spec (acfc2fa) > - bigtop-packages/src/rpm/oozie/SPECS/oozie.spec (4bbc212) > - bigtop-packages/src/rpm/solr/SOURCES/solr-server.init (6d1a035) > - bigtop-packages/src/rpm/sqoop/SPECS/sqoop.spec (5f4447b) > > View Diff <https://reviews.apache.org/r/12526/diff/> >
