GitHub PullRequestReviewCommentEvent at 2014-04-06 19:10:46
Repository: spring-xd
Actor: markpollack
Payload: {"comment":{"url":"https://api.github.com/repos/spring-projects/spring-xd/pulls/comments/11327545","id":11327545,"diff_hunk":"@@ -0,0 +1,182 @@\n+#!/usr/bin/env bash\n+\n+##############################################################################\n+##\n+##  hsqldb-server start up script for UN*X\n+##\n+##############################################################################\n+\n+# Add default JVM options here. You can also use JAVA_OPTS to pass JVM options to this script.\n+DEFAULT_JVM_OPTS=\"\"\n+\n+APP_NAME=\"hsqldb-server\"\n+APP_BASE_NAME=`basename \"$0\"`\n+\n+# Use the maximum available, or set MAX_FD != -1 to use that value.\n+MAX_FD=\"maximum\"\n+\n+warn ( ) {\n+    echo \"$*\"\n+}\n+\n+die ( ) {\n+    echo\n+    echo \"$*\"\n+    echo\n+    exit 1\n+}\n+\n+# OS specific support (must be 'true' or 'false').\n+cygwin=false\n+msys=false\n+darwin=false\n+case \"`uname`\" in\n+  CYGWIN* )\n+    cygwin=true\n+    ;;\n+  Darwin* )\n+    darwin=true\n+    ;;\n+  MINGW* )\n+    msys=true\n+    ;;\n+esac\n+\n+# For Cygwin, ensure paths are in UNIX format before anything is touched.\n+if $cygwin ; then\n+    [ -n \"$JAVA_HOME\" ] && JAVA_HOME=`cygpath --unix \"$JAVA_HOME\"`\n+fi\n+\n+# Attempt to set APP_HOME\n+# Resolve links: $0 may be a link\n+PRG=\"$0\"\n+# Need this for relative symlinks.\n+while [ -h \"$PRG\" ] ; do\n+    ls=`ls -ld \"$PRG\"`\n+    link=`expr \"$ls\" : '.*-> \\(.*\\)$'`\n+    if expr \"$link\" : '/.*' > /dev/null; then\n+        PRG=\"$link\"\n+    else\n+        PRG=`dirname \"$PRG\"`\"/$link\"\n+    fi\n+done\n+SAVED=\"`pwd`\"\n+cd \"`dirname \\\"$PRG\\\"`/..\" >&-\n+APP_HOME=\"`pwd -P`\"\n+cd \"$SAVED\" >&-\n+\n+APP_HOME_LIB=$APP_HOME/lib\n+if [ -d \"$APP_HOME_LIB\" ]; then\n+    for i in \"$APP_HOME_LIB\"/*.jar; do\n+        CLASSPATH=\"$CLASSPATH\":\"$i\"\n+    done\n+fi\n+\n+# Determine the Java command to use to start the JVM.\n+if [ -n \"$JAVA_HOME\" ] ; then\n+    if [ -x \"$JAVA_HOME/jre/sh/java\" ] ; then\n+        # IBM's JDK on AIX uses strange locations for the executables\n+        JAVACMD=\"$JAVA_HOME/jre/sh/java\"\n+    else\n+        JAVACMD=\"$JAVA_HOME/bin/java\"\n+    fi\n+    if [ ! -x \"$JAVACMD\" ] ; then\n+        die \"ERROR: JAVA_HOME is set to an invalid directory: $JAVA_HOME\n+\n+Please set the JAVA_HOME variable in your environment to match the\n+location of your Java installation.\"\n+    fi\n+else\n+    JAVACMD=\"java\"\n+    which java >/dev/null 2>&1 || die \"ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.\n+\n+Please set the JAVA_HOME variable in your environment to match the\n+location of your Java installation.\"\n+fi\n+\n+# Increase the maximum file descriptors if we can.\n+if [ \"$cygwin\" = \"false\" -a \"$darwin\" = \"false\" ] ; then\n+    MAX_FD_LIMIT=`ulimit -H -n`\n+    if [ $? -eq 0 ] ; then\n+        if [ \"$MAX_FD\" = \"maximum\" -o \"$MAX_FD\" = \"max\" ] ; then\n+            MAX_FD=\"$MAX_FD_LIMIT\"\n+        fi\n+        ulimit -n $MAX_FD\n+        if [ $? -ne 0 ] ; then\n+            warn \"Could not set maximum file descriptor limit: $MAX_FD\"\n+        fi\n+    else\n+        warn \"Could not query maximum file descriptor limit: $MAX_FD_LIMIT\"\n+    fi\n+fi\n+\n+# For Darwin, add options to specify how the application appears in the dock\n+if $darwin; then\n+    GRADLE_OPTS=\"$GRADLE_OPTS \\\"-Xdock:name=$APP_NAME\\\" \\\"-Xdock:icon=$APP_HOME/media/gradle.icns\\\"\"\n+fi\n+\n+# For Cygwin, switch paths to Windows format before running java\n+if $cygwin ; then\n+    APP_HOME=`cygpath --path --mixed \"$APP_HOME\"`\n+    CLASSPATH=`cygpath --path --mixed \"$CLASSPATH\"`\n+\n+    # We build the pattern for arguments to be converted via cygpath\n+    ROOTDIRSRAW=`find -L / -maxdepth 1 -mindepth 1 -type d 2>/dev/null`\n+    SEP=\"\"\n+    for dir in $ROOTDIRSRAW ; do\n+        ROOTDIRS=\"$ROOTDIRS$SEP$dir\"\n+        SEP=\"|\"\n+    done\n+    OURCYGPATTERN=\"(^($ROOTDIRS))\"\n+    # Add a user-defined pattern to the cygpath arguments\n+    if [ \"$GRADLE_CYGPATTERN\" != \"\" ] ; then\n+        OURCYGPATTERN=\"$OURCYGPATTERN|($GRADLE_CYGPATTERN)\"\n+    fi\n+    # Now convert the arguments - kludge to limit ourselves to /bin/sh\n+    i=0\n+    for arg in \"$@\" ; do\n+        CHECK=`echo \"$arg\"|egrep -c \"$OURCYGPATTERN\" -`\n+        CHECK2=`echo \"$arg\"|egrep -c \"^-\"`                                 ### Determine if an option\n+\n+        if [ $CHECK -ne 0 ] && [ $CHECK2 -eq 0 ] ; then                    ### Added a condition\n+            eval `echo args$i`=`cygpath --path --ignore --mixed \"$arg\"`\n+        else\n+            eval `echo args$i`=\"\\\"$arg\\\"\"\n+        fi\n+        i=$((i+1))\n+    done\n+    case $i in\n+        (0) set -- ;;\n+        (1) set -- \"$args0\" ;;\n+        (2) set -- \"$args0\" \"$args1\" ;;\n+        (3) set -- \"$args0\" \"$args1\" \"$args2\" ;;\n+        (4) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" ;;\n+        (5) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" \"$args4\" ;;\n+        (6) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" \"$args4\" \"$args5\" ;;\n+        (7) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" \"$args4\" \"$args5\" \"$args6\" ;;\n+        (8) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" \"$args4\" \"$args5\" \"$args6\" \"$args7\" ;;\n+        (9) set -- \"$args0\" \"$args1\" \"$args2\" \"$args3\" \"$args4\" \"$args5\" \"$args6\" \"$args7\" \"$args8\" ;;\n+    esac\n+fi\n+\n+# check if XD_HOME is already set; if it is not, then set APP_HOME as XD_HOME\n+if [ x\"$XD_HOME\" = x ] ; then\n+    export XD_HOME=$APP_HOME/../xd\n+fi\n+\n+# Check for an explicity set XD_CONFIG\n+if [ x\"$XD_CONFIG\" = x ] ; then\n+    export XD_CONFIG=file:$XD_HOME/config/xd-config.yml","path":"scripts/hsqldb/hsqldb-server","position":169,"original_position":169,"commit_id":"0c25e311d0820013825926cb8f8d114d2232d386","original_commit_id":"0c25e311d0820013825926cb8f8d114d2232d386","user":{"login":"markpollack","id":247466,"avatar_url":"https://avatars.githubusercontent.com/u/247466?","gravatar_id":"3c2bbcb3e173ee1b0c2a8bdb2e293633","url":"https://api.github.com/users/markpollack","html_url":"https://github.com/markpollack","followers_url":"https://api.github.com/users/markpollack/followers","following_url":"https://api.github.com/users/markpollack/following{/other_user}","gists_url":"https://api.github.com/users/markpollack/gists{/gist_id}","starred_url":"https://api.github.com/users/markpollack/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/markpollack/subscriptions","organizations_url":"https://api.github.com/users/markpollack/orgs","repos_url":"https://api.github.com/users/markpollack/repos","events_url":"https://api.github.com/users/markpollack/events{/privacy}","received_events_url":"https://api.github.com/users/markpollack/received_events","type":"User","site_admin":false},"body":"Why is XD_CONFIG used here, in particular as it is set to a file vs. a path and also XD_CONFIG itself is no longer used in xd-singlenode/xd-container - instead a pair of variables that mimic the naming convention of boot is used, namely XD_CONFIG_NAME and XD_CONFIG_LOCATION.  \r\n\r\nThat said, I don't see why there is a need to reference any XD_<xyz> environment variable, the only place I see anything related to XD_HOME is in hsql-datasource.xml\r\n\r\n\t\t\t<property name=\"serverProperties\">\r\n\t\t\t\t<props>\r\n\t\t\t\t\t<prop key=\"server.port\">${hsql.server.port:9101}</prop>\r\n\t\t\t\t\t<prop key=\"server.database.0\">${xd.data.home:file:${XD_HOME:..}/data}/jobs/${hsql.server.database:xdjobrepo}\r\n\t\t\t\t\t</prop>\r\n\t\t\t\t\t<prop key=\"server.dbname.0\">${hsql.server.dbname:xdjob}</prop>\r\n\t\t\t\t</props>\r\n\t\t\t</property>\r\n\r\nI think the database file location when running in embedded mode in the singlenode sever should be different than when running standalone hsqldb server.  What if we use a variable `hsql.server.data.location` as the variable name for the property server.database.0  ?\r\n\r\nThe startup script for the database would then not even have to reference any extra XD_ config variables, when starting hsqldb-server `hsql.server.data.location` can be set to APP_HOME/data.  I don't think we even need to set spring.config.location or spring.config.name as -D options in the hsqldb-server startup script.  For the case of the singlenode server hsql.server.data.location could be set to $(XD_HOME:..}/data by default in dirt's application.yml and if someone wanted to override it, they would in xd-config.yml or environment vars etc.","created_at":"2014-04-06T19:10:46Z","updated_at":"2014-04-06T19:10:46Z","html_url":"https://github.com/spring-projects/spring-xd/pull/699#discussion_r11327545","pull_request_url":"https://api.github.com/repos/spring-projects/spring-xd/pulls/699","_links":{"self":{"href":"https://api.github.com/repos/spring-projects/spring-xd/pulls/comments/11327545"},"html":{"href":"https://github.com/spring-projects/spring-xd/pull/699#discussion_r11327545"},"pull_request":{"href":"https://api.github.com/repos/spring-projects/spring-xd/pulls/699"}}}}