You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/01/25 01:52:12 UTC
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12271
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* TODO: complete exhaustive run
* TODO: run on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 160 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/3
--
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: newchange
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12271/5/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/12271/5/bin/start-impala-cluster.py@150
PS5, Line 150: minicluster
I think 'minicluster' means the non-impala parts like HDFS, Hive, HBase, etc. If so, can we change it to 'run_impala_daemon'?
--
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: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 13:41:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Remove some options like -jvm_suspend, -gdb, -perf that
may not be used. These can be reintroduced if needed.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* Ran exhaustive tests.
* Ran on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Reviewed-on: http://gerrit.cloudera.org:8080/12271
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 157 insertions(+), 339 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12271
to look at the new patch set (#9).
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Remove some options like -jvm_suspend, -gdb, -perf that
may not be used. These can be reintroduced if needed.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* Ran exhaustive tests.
* Ran on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 157 insertions(+), 339 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/9
--
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: newpatchset
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12271
to look at the new patch set (#6).
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* Ran exhaustive tests.
* TODO: run on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 161 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/6
--
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: newpatchset
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3714/ DRY_RUN=false
--
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: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 08:55:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1934/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 20:16:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 7:
(8 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: sanitis
> Is this supposed to be sanitizer?
Done
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 ca
Yeah I originally preserved that behaviour but realised it was redundant with setting -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 i
I don't really use the debugger functionality extensively and it doesn't seem like it was possible to set this from start-impala-cluster.py, so I don't know how likely it is that it was in use.
I figured in some of these cases it was better to remove the functionality and add it back if someone noticed it missing rather than trying to port everything.
http://gerrit.cloudera.org:8080/#/c/12271/6/bin/start-impala-cluster.py@372
PS6, Line 372:
> Nit: stray line
Done
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: output_file=None
> Do we use the tool_prefix anywhere? I see that this replaces "-perf" and "-
Yeah I guess it isn't really usable. I'll just remove it for now. I don't think it's too hard to add back in if someone misses it.
http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@497
PS6, Line 497:
> Nit: stray "/" ?
Done
http://gerrit.cloudera.org:8080/#/c/12271/6/tests/common/impala_cluster.py@498
PS6, Line 498:
> One thing I noticed is that gdb would need to run with the "--args" option
Removed tool_prefix.
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 '-'
Done. This is why I don't like fancy vertical alignment in code - it's just fussy to change it.
--
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: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 13:59:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1930/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 18:46:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12271
to look at the new patch set (#7).
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Remove some options like -jvm_suspend, -gdb, -perf that
may not be used. These can be reintroduced if needed.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* Ran exhaustive tests.
* TODO: run on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 157 insertions(+), 339 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/7
--
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: newpatchset
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12271
to look at the new patch set (#5).
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* Ran exhaustive tests.
* TODO: run on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 161 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/5
--
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: newpatchset
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 9: Code-Review+2
This looks good to me. Thanks for taking this on!
--
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: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 02:48:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 10: Verified+1
--
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: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 13:10:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12271
to look at the new patch set (#4).
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
IMPALA-7999: clean up start-*d.sh scripts
Delete these wrapper scripts and replace with a generic
start-daemon.sh script that sets environment variables
without the other logic.
Move the logic for setting JAVA_TOOL_OPTIONS into
start-impala-cluster.py.
Port across the kerberized minicluster logic (which has
probably bitrotted) in case it needs to be revived.
Remove --verbose option that didn't appear to be useful
(it claims to print daemon output to the console,
but output is still redirected regardless).
Removed a level of quoting in custom cluster test argument
handling - this was made unnecessary by properly escaping
arguments with pipes.escape() in run_daemon().
Testing:
* TODO: complete exhaustive run
* TODO: run on CentOS 6 to confirm we didn't reintroduce Popen issue
worked around by kwho.
Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
---
D bin/start-catalogd.sh
A bin/start-daemon.sh
M bin/start-impala-cluster.py
D bin/start-impalad.sh
D bin/start-statestored.sh
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_cluster.py
M tests/custom_cluster/test_breakpad.py
M tests/custom_cluster/test_redaction.py
M tests/custom_cluster/test_scratch_disk.py
10 files changed, 161 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/12271/4
--
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: newpatchset
Gerrit-Change-Id: Ib67444fd4def8da119db5d3a0832ef1de15b068b
Gerrit-Change-Number: 12271
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 10: Code-Review+2
--
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: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 08:55:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1958/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Feb 2019 15:29:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1884/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Jan 2019 02:32:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Jan 2019 19:35:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7999: clean up start-*d.sh scripts
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12271 )
Change subject: IMPALA-7999: clean up start-*d.sh scripts
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12271/5/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/12271/5/bin/start-impala-cluster.py@150
PS5, Line 150: minicluster
> I think 'minicluster' means the non-impala parts like HDFS, Hive, HBase, et
I renamed to run_daemon_with_options(), which I think is a more accurate description of what it does.
--
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: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 14:24:47 +0000
Gerrit-HasComments: Yes