You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/10/15 18:56:58 UTC
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11687
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
This patch adds two options start-impala-cluster.py:
--restart_catalogd_only to restart catalogd process
--restart_statestored_only to restart statestored process
Testing:
- Manually tested the two new options
Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
---
M bin/start-impala-cluster.py
1 file changed, 28 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11687/1
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py@68
PS1, Line 68: parser.add_option("-r", "--restart_impalad_only", dest="restart_impalad_only",
: action="store_true", default=False,
: help="Restarts only the impalad processes")
: parser.add_option("--restart_catalogd_only", dest="restart_catalogd_only",
: action="store_true", default=False,
: help="Restarts only the catalogd process")
: parser.add_option("--restart_statestored_only", dest="restart_statestored_only",
: action="store_true", default=False,
: help="Restarts only the statestored process")
: parser.add_option("--in-process", dest="inprocess", action="store_true", default=False,
: help="Start all Impala backends and state store in a single process.")
> These four options are mutually exclusive. Sadly, it looks like https://doc
Done
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py@477
PS1, Line 477: if options.inprocess:
: LOG.error(
: "Cannot p
> I think you can make this slightly clearer by making the 3 "only" options a
Done
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 19:50:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
This patch adds two options start-impala-cluster.py:
--restart_catalogd_only to restart catalogd process
--restart_statestored_only to restart statestored process
Testing:
- Manually tested the two new options
Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
---
M bin/start-impala-cluster.py
1 file changed, 39 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11687/2
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3316/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 21:06:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 21:06:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
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/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
This patch adds two options start-impala-cluster.py:
--restart_catalogd_only to restart catalogd process
--restart_statestored_only to restart statestored process
Testing:
- Manually tested the two new options
Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Reviewed-on: http://gerrit.cloudera.org:8080/11687
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M bin/start-impala-cluster.py
1 file changed, 32 insertions(+), 6 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Oct 2018 01:23:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py@164
PS2, Line 164: def kill_cluster_processes(binaries=["catalogd", "impalad", "statestored"], force=False,
This probably works, but it's bad because if someone mutates the default list, all hell breaks loose.
See https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html for example.
Anyway, you should use binaries=None here and then handle that somehow...
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py@476
PS2, Line 476: def in_process_check():
Just add options.inprocess to your list in line 470. It's exclusive of all of those other options. Then you don't need this pre_kill_check business.
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 20:16:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
This patch adds two options start-impala-cluster.py:
--restart_catalogd_only to restart catalogd process
--restart_statestored_only to restart statestored process
Testing:
- Manually tested the two new options
Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
---
M bin/start-impala-cluster.py
1 file changed, 32 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11687/3
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
Seems fine to me. I think you could tighten up the argument parsing to do something like:
if len([ x for x in [options.impalad_only, options.catalogd_only, ...] if x ]) > 1:
LOG.error("these options are mutually exclusive")
...
but I'm not totally convinced it's terser or clearer than what you have.
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py@68
PS1, Line 68: parser.add_option("-r", "--restart_impalad_only", dest="restart_impalad_only",
: action="store_true", default=False,
: help="Restarts only the impalad processes")
: parser.add_option("--restart_catalogd_only", dest="restart_catalogd_only",
: action="store_true", default=False,
: help="Restarts only the catalogd process")
: parser.add_option("--restart_statestored_only", dest="restart_statestored_only",
: action="store_true", default=False,
: help="Restarts only the statestored process")
: parser.add_option("--in-process", dest="inprocess", action="store_true", default=False,
: help="Start all Impala backends and state store in a single process.")
These four options are mutually exclusive. Sadly, it looks like https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group isn't available in optparse...
http://gerrit.cloudera.org:8080/#/c/11687/1/bin/start-impala-cluster.py@477
PS1, Line 477: LOG.error(
: "Cannot perform individual component restarts using an in-process cluster")
: sys.exit(1)
I think you can make this slightly clearer by making the 3 "only" options and the "in-process" explicitly mutually exclusive and then removing the "options.inprocess" clause that's replicated three times.
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 19:27:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py@164
PS2, Line 164: def kill_cluster_processes(force=False):
> This probably works, but it's bad because if someone mutates the default li
I decided to not modify this function.
http://gerrit.cloudera.org:8080/#/c/11687/2/bin/start-impala-cluster.py@476
PS2, Line 476: if options.inprocess:
> Just add options.inprocess to your list in line 470. It's exclusive of all
Done
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 20:48:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11687 )
Change subject: IMPALA-7709: Add options to restart catalogd and statestored in start-impala-cluster.py
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide26902f6bce11718708d5ab0174282dd94400a3
Gerrit-Change-Number: 11687
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 21:00:50 +0000
Gerrit-HasComments: No