You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by shivzone <gi...@git.apache.org> on 2017/11/02 22:48:15 UTC

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

GitHub user shivzone opened a pull request:

    https://github.com/apache/incubator-hawq/pull/1306

    HAWQ-1543. Make pxf configurable upon restart through pxf-env

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/shivzone/incubator-hawq HAWQ-1543

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-hawq/pull/1306.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1306
    
----
commit 23c8b6864698fc05fe343774e6885bd284d4db4a
Author: shivzone <sh...@gmail.com>
Date:   2017-11-02T22:47:34Z

    HAWQ-1543. Make pxf configurable upon restart through pxf-env

----


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148685123
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-service ---
    @@ -249,6 +206,34 @@ function patchWebapp()
         cat $web_file | \
         sed "s:<param-value>.*pxf-log4j.properties<\/param-value>:<param-value>$PXF_HOME\/conf\/pxf-log4j.properties<\/param-value>:" > web.xml.tmp
         mv web.xml.tmp $web_file
    +
    +    # set port
    +    catalinaProperties=$instance_root/$instance_name/conf/catalina.properties
    +    cat $catalinaProperties | \
    +    sed "s|^[[:blank:]]*connector.http.port=.*$|connector.http.port=$instance_port|g" \
    +    > ${catalinaProperties}.tmp
    +
    +    rm $catalinaProperties
    +    mv ${catalinaProperties}.tmp $catalinaProperties
    --- End diff --
    
    can we just copy from a template and "sed" in place instead of all these file manipulations ?


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148694457
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-env.sh ---
    @@ -41,6 +41,9 @@ fi
     # Port
     export PXF_PORT=${PXF_PORT:-51200}
     
    +# Memory
    +export PXF_HEAP_OPTS="-Xmx2g -Xms1g"
    --- End diff --
    
    will change


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148685296
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-env.sh ---
    @@ -41,6 +41,9 @@ fi
     # Port
     export PXF_PORT=${PXF_PORT:-51200}
     
    +# Memory
    +export PXF_HEAP_OPTS="-Xmx2g -Xms1g"
    --- End diff --
    
    I would call these PXF_JVM_OPTS since there can be other options here (like GC setting, etc)


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148838659
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-service ---
    @@ -216,17 +174,16 @@ function doInit()
     	determineHadoopDistro
     	generatePrivateClasspath || return 1
     	createInstance || return 1
    -	configureInstance || return 1
     	deployWebapp || return 1
     	createLogsDir || return 1
     	createRunDir  || return 1
     }
     
     #
    -# patchWebapp patches the webapp config files
    -# patch applied only if PXF_HOME is defined
    +# configureWebapp patches the webapp with pxf and user overriden configs
    +# applied only if PXF_HOME is defined
     #
    -function patchWebapp()
    +function configureWebapp()
    --- End diff --
    
    The purpose of patching the war is so that an update done to PXF as part of upgrades wouldn't require the user to run pxf init again. They can simply use pxf restart.
    Can evaluate that change outside of this PR


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148684991
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-service ---
    @@ -216,17 +174,16 @@ function doInit()
     	determineHadoopDistro
     	generatePrivateClasspath || return 1
     	createInstance || return 1
    -	configureInstance || return 1
     	deployWebapp || return 1
     	createLogsDir || return 1
     	createRunDir  || return 1
     }
     
     #
    -# patchWebapp patches the webapp config files
    -# patch applied only if PXF_HOME is defined
    +# configureWebapp patches the webapp with pxf and user overriden configs
    +# applied only if PXF_HOME is defined
     #
    -function patchWebapp()
    +function configureWebapp()
    --- End diff --
    
    since this function is now called on every start, we should not be unpacking WAR file and edit web.xml -- only catalina files that use dynamic properties should be regenerated. Ideally, they should be regenerated from templates that do not get destroyed, so that customers have a chance to change the template, if needed. 


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by lavjain <gi...@git.apache.org>.
Github user lavjain commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1306#discussion_r148859336
  
    --- Diff: pxf/pxf-service/src/scripts/pxf-service ---
    @@ -249,6 +206,34 @@ function patchWebapp()
         cat $web_file | \
         sed "s:<param-value>.*pxf-log4j.properties<\/param-value>:<param-value>$PXF_HOME\/conf\/pxf-log4j.properties<\/param-value>:" > web.xml.tmp
         mv web.xml.tmp $web_file
    +
    +    # set port
    +    catalinaProperties=$instance_root/$instance_name/conf/catalina.properties
    +    cat $catalinaProperties | \
    +    sed "s|^[[:blank:]]*connector.http.port=.*$|connector.http.port=$instance_port|g" \
    +    > ${catalinaProperties}.tmp
    +
    +    rm $catalinaProperties
    +    mv ${catalinaProperties}.tmp $catalinaProperties
    --- End diff --
    
    sed -i would be preferred as Alex suggested.


---

[GitHub] incubator-hawq pull request #1306: HAWQ-1543. Make pxf configurable upon res...

Posted by shivzone <gi...@git.apache.org>.
Github user shivzone closed the pull request at:

    https://github.com/apache/incubator-hawq/pull/1306


---