You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bigtop.apache.org by Sean Mackrory <ma...@gmail.com> on 2013/07/13 00:16:54 UTC
Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more dynamic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12526/
-----------------------------------------------------------
Review request for bigtop and Mark Grover.
Bugs: BIGTOP-939
https://issues.apache.org/jira/browse/BIGTOP-939
Repository: bigtop
Description
-------
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
Diff: https://reviews.apache.org/r/12526/diff/
Testing
-------
Explained on JIRA - uploaded here only for commenting convenience.
Thanks,
Sean Mackrory
Re: Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat
more dynamic
Posted by Sean Mackrory <ma...@gmail.com>.
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 <ma...@gmail.com> 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/>
>
Re: Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more
dynamic
Posted by Sean Mackrory <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12526/#review23156
-----------------------------------------------------------
bigtop-packages/src/common/hadoop/install_hadoop.sh
<https://reviews.apache.org/r/12526/#comment47007>
Good catch - does look like it should be. Will fix...
bigtop-packages/src/common/oozie/install_oozie.sh
<https://reviews.apache.org/r/12526/#comment47008>
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/#comment47010>
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/#comment47006>
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/#comment47003>
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/#comment47005>
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/#comment47004>
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/#comment47009>
Fixed. Should have no effect, though.
- Sean Mackrory
On July 12, 2013, 10:16 p.m., Sean Mackrory wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12526/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 10:16 p.m.)
>
>
> Review request for bigtop and Mark Grover.
>
>
> Bugs: BIGTOP-939
> https://issues.apache.org/jira/browse/BIGTOP-939
>
>
> Repository: bigtop
>
>
> Description
> -------
>
> 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
>
> Diff: https://reviews.apache.org/r/12526/diff/
>
>
> Testing
> -------
>
> Explained on JIRA - uploaded here only for commenting convenience.
>
>
> Thanks,
>
> Sean Mackrory
>
>
Re: Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more
dynamic
Posted by Mark Grover <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12526/#review23105
-----------------------------------------------------------
bigtop-packages/src/common/hadoop/install_hadoop.sh
<https://reviews.apache.org/r/12526/#comment46932>
Should this be
chmod 644 $HTTPFS_ETC_DIR/conf.empty/tomcat-deployment/conf/*?
bigtop-packages/src/common/oozie/install_oozie.sh
<https://reviews.apache.org/r/12526/#comment46937>
The old code shows that we need to change 2 xml files for ssl configuration - web.xml and server.xml.
You only seem to be changing web.xml. Should we change server.xml as well here (you seem to be changing that in the init script)?
bigtop-packages/src/common/oozie/install_oozie.sh
<https://reviews.apache.org/r/12526/#comment46941>
Should we have a default webapps symlink (pointing to the non-SSL configuration) out of the box? It would get copied over when we do cp -r in the init script.
bigtop-packages/src/common/oozie/oozie.init
<https://reviews.apache.org/r/12526/#comment46945>
I think we should forcefully remove the symlink to be on the safe side before we create it. Alternatively, we can force create the symlink, overriding the previous symlink.
bigtop-packages/src/common/oozie/oozie.init
<https://reviews.apache.org/r/12526/#comment46948>
Ok, so how does this work if someone wants to use their own custom configuration for oozie? Historically, we have provided that option so all users have to do is to update the alternatives symlink to their own custom config directory but this doesn't seem to be in line with that.
bigtop-packages/src/common/sqoop/sqoop-server.sh
<https://reviews.apache.org/r/12526/#comment46965>
extra prepending slash before ${DEPLOYMENT_TARGET}?
- Mark Grover
On July 12, 2013, 10:16 p.m., Sean Mackrory wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12526/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 10:16 p.m.)
>
>
> Review request for bigtop and Mark Grover.
>
>
> Bugs: BIGTOP-939
> https://issues.apache.org/jira/browse/BIGTOP-939
>
>
> Repository: bigtop
>
>
> Description
> -------
>
> 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
>
> Diff: https://reviews.apache.org/r/12526/diff/
>
>
> Testing
> -------
>
> Explained on JIRA - uploaded here only for commenting convenience.
>
>
> Thanks,
>
> Sean Mackrory
>
>
Re: Review Request 12526: BIGTOP-939. Make usage of bigtop-tomcat more
dynamic
Posted by Mark Grover <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12526/#review23122
-----------------------------------------------------------
bigtop-packages/src/common/oozie/oozie.init
<https://reviews.apache.org/r/12526/#comment46970>
The reason I say this is because we are referring to conf directory here. We delivered the conf.dist directory through packages. Conf dir can technically be pointing to anything. I may just be overthinking this:-)
- Mark Grover
On July 12, 2013, 10:16 p.m., Sean Mackrory wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12526/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 10:16 p.m.)
>
>
> Review request for bigtop and Mark Grover.
>
>
> Bugs: BIGTOP-939
> https://issues.apache.org/jira/browse/BIGTOP-939
>
>
> Repository: bigtop
>
>
> Description
> -------
>
> 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
>
> Diff: https://reviews.apache.org/r/12526/diff/
>
>
> Testing
> -------
>
> Explained on JIRA - uploaded here only for commenting convenience.
>
>
> Thanks,
>
> Sean Mackrory
>
>