----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22845/#review46549 -----------------------------------------------------------
samza-shell/src/main/bash/run-class.sh <https://reviews.apache.org/r/22845/#comment81991> When I read the name "SAMZA_LOG4J_FILE", I first thought this was going to be the file that logs are written to, not the file containing the configuration. Could we call it SAMZA_LOG4J_CONFIG instead? Also, just wanted to point out that we've lost the check for whether the config file exists. I don't think that's a problem (log4j will be unconfigured either way). samza-shell/src/main/bash/run-job.sh <https://reviews.apache.org/r/22845/#comment81993> Perhaps call it log4j-console.xml? "default" is kinda generic and unspecific. This log4j config gets used when a tool is invoked through the shell script, but not when invoked through Gradle (e.g. `./gradlew samza-shell:checkpointTool`). The gradle run classpath currently includes slf4j-simple, which also logs to the console, but with different formatting. Do you think you might be able to make the Gradle tasks use the same logging config as the shell scripts? samza-shell/src/main/resources/log4j-default.xml <https://reviews.apache.org/r/22845/#comment82023> Irregular indentation (5 spaces) - Martin Kleppmann On June 20, 2014, 10:19 p.m., Yan Fang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22845/ > ----------------------------------------------------------- > > (Updated June 20, 2014, 10:19 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > 1. added an environment variable SAMZA_LOG4J_FILE in each script. and > run-class.sh use that variable for log4j configuration instead of directly > pointing to the position of log4j.xml. > 2. added log4j-default.xml, which append the logs to console. run-job, > kill-yarn-job and checkpoint scripts use this configuration. > 3. changed the build.gradle to contain the new log4j-default.xml > > > Diffs > ----- > > build.gradle 1bbd5ca > samza-shell/src/main/bash/checkpoint-tool.sh 1a455df > samza-shell/src/main/bash/kill-yarn-job.sh 924aac6 > samza-shell/src/main/bash/run-am.sh c202596 > samza-shell/src/main/bash/run-class.sh cc341b7 > samza-shell/src/main/bash/run-container.sh 72cee18 > samza-shell/src/main/bash/run-job.sh 0605994 > samza-shell/src/main/resources/log4j-default.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/22845/diff/ > > > Testing > ------- > > > Thanks, > > Yan Fang > >
