Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13313 )
Change subject: IMPALA-6042: Allow Impala shell to use a global impalarc config ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell.py@1624 PS1, Line 1624: user_config = impala_shell_defaults.get("config_file") : global_config = impala_shell_defaults.get("global_config") we should call these variables. default_user_config and default_global_config http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell.py@1628 PS1, Line 1628: # use path to custom file specified by user in config_file option : input_config = os.path.expanduser(options.config_file) : # for testing: use path to custom global config : test_global_config = os.path.expanduser(options.global_config) : : if os.path.isfile(test_global_config) and test_global_config != global_config: : configs_to_load[0] = test_global_config : elif test_global_config != global_config: : print_to_stderr('%s not found.\n' % test_global_config) : sys.exit(1) : : # verify input_config, if found : if os.path.isfile(input_config) and input_config != user_config: : if options.verbose: : print_to_stderr("Loading in options from config file: %s \n" % input_config) : # Command line overrides loading ~/.impalarc : configs_to_load[1] = input_config : elif input_config != user_config: : print_to_stderr('%s not found.\n' % input_config) : sys.exit(1) The logic here is difficult to follow. I believe this is basically what we want: if input config is specified: check if the input file exists then use it else: use the one from the default I think we should try to simplify the logic a bit. http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell_config_defaults.py File shell/impala_shell_config_defaults.py: http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell_config_defaults.py@55 PS1, Line 55: .impalarc Usually files in /etc are not dot files. So /etc/impalarc is better. http://gerrit.cloudera.org:8080/#/c/13313/1/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/13313/1/shell/option_parser.py@260 PS1, Line 260: help=SUPPRESS_HELP why do we suppress help here? -- To view, visit http://gerrit.cloudera.org:8080/13313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a3179b6d9c9e3b2b01d6d3c5847cadb68782816 Gerrit-Change-Number: 13313 Gerrit-PatchSet: 1 Gerrit-Owner: Ethan Xue <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Sat, 11 May 2019 00:43:28 +0000 Gerrit-HasComments: Yes
