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 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/13313/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13313/3//COMMIT_MSG@9 PS3, Line 9: Currently, impalarc files can be specified on a per-user basis (stored in ~/.impalarc), and they aren't created by default. The Impala shell should pick up /etc/impalarc as well, in addition to the user-specific configurations. : : The intent here is to allow a "global" configuration of the shell by a system administrator. nit: we should try to keep it within 72-char width 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@1628 PS1, Line 1628: test_global_config = os.path.expanduser(options.global_config) : if os.path.isfile(test_global_config): : # Always output the source of the global config if verbose : if options.verbose: : print_to_stderr( : "Loading in options from global config file: %s \n" % test_global_config) : if test_global_config != default_global_config: : default_global_config = test_global_config : elif test_global_config != default_global_config: : print_to_stderr('%s not found.\n' % test_global_config) : sys.exit(1) : : # use path to custom file specified by user in config_file option : input_config = os.path.expanduser(options.config_file) : # verify input_config, if found : if os.path.isfile(input_config) and input_config != default_user_config: : if options.verbose: : print_to_stderr("Loading in options from config file: %s \n" % input_config) : # command line overrides loading ~/.impalarc : default_use > Thanks for the heads up. I tried my best to make it more readable. It does One confusion I had earlier was particularly this line: input_config != user_config: to mean file not found. I think this can be made more readable like below: if input_config != user_config: if os.path.isfile(input_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 else: print_to_stderr('%s not found.\n' % input_config) sys.exit(1) Now it's fairly easy to tell that the "else" means file not found :) http://gerrit.cloudera.org:8080/#/c/13313/1/shell/impala_shell.py@1656 PS1, Line 1656: nit: formatting is a bit odd. In Python, we usually indent parser.option_list to be in the same col as config_file. s_options, q_options = get_config_from_file(config_file, parser.option_list) 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 > The global config option is only used for testing purposes, so users should One potential use case that maybe useful is is to specify a different location of global config than /etc/impalarc. And currently there's no way to do this with this CR other than using --global_config that's supposed to be a hidden flag. Furthermore, having --global_config as an impala shell is kinda redundant especially since we already have --config_file. I think a better way is to use an environment variable, e.g. IMPALA_CONFIG_FILE. With this, if we want to use a different global config file than the default /etc/impalarc, users can specify the IMPALA_CONFIG_FILE in /etc/profile or something of that sort. If IMPALA_CONFIG_FILE is not present, it will simply use the /etc/impalarc. This has benefits of being able to specify a different global config file as well as making testing easier. What do you think? -- 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: 3 Gerrit-Owner: Ethan Xue <ethan....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Ethan Xue <ethan....@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 16 May 2019 00:32:51 +0000 Gerrit-HasComments: Yes
