Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-daemon.sh File bin/start-daemon.sh: http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-daemon.sh@31 PS6, Line 31: saniser Is this supposed to be sanitizer? http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-impala-cluster.py@160 PS6, Line 160: java_tool_options = "" The start-* scripts put the JAVA_TOOL_OPTIONS from the env at the end in case they override the ones we generate. We might want to preserve that behavior. I guess the appropriate path is to specify the jvm_args. http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-impala-cluster.py@163 PS6, Line 163: suspend=n This is getting rid of the suspend=y option. I can't think of a time when it has been useful, so I think that is mostly good. http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-impala-cluster.py@372 PS6, Line 372: Nit: stray line http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py File tests/common/impala_cluster.py: http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@493 PS6, Line 493: tool_prefix=None Do we use the tool_prefix anywhere? I see that this replaces "-perf" and "-gdb". How would one use it? http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@493 PS6, Line 493: args Can we add a comment somewhere saying that "args" is a list of strings? http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@497 PS6, Line 497: / Nit: stray "/" ? http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@498 PS6, Line 498: ["gdb"] One thing I noticed is that gdb would need to run with the "--args" option (see start-impalad.sh). Maybe it should be listed here in the comment, maybe not (as long as it is incorporated into the tool_prefix somewhere). http://gerrit.cloudera.org:8080/#/c/12271/6/tests/custom_cluster/test_redaction.py File tests/custom_cluster/test_redaction.py: http://gerrit.cloudera.org:8080/#/c/12271/6/tests/custom_cluster/test_redaction.py@95 PS6, Line 95: -audit_event_log_dir=%s : -profile_log_dir=%s : -redaction_rules_file=%s : -vmodule=%s""" Nit: line up the '-' -- To view, visit http://gerrit.cloudera.org:8080/12271 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b Gerrit-Change-Number: 12271 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 31 Jan 2019 19:35:00 +0000 Gerrit-HasComments: Yes
