Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 )
Change subject: IMPALA-8330: Impala shell config file should use flag names ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@23 PS2, Line 23: Q > It's just an additional feature that we support. I agree that users should Okay. For a second, I forgot that the options parser supported short opts. So I got confused. http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@67 PS2, Line 67: contains short and long options for a quick lookup. also mention it maps to the dest? http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@275 PS2, Line 275: short_opt = option._short_opts[0][1:] if len(option._short_opts) > 0 else None I had to print these stuff locally to understand whats going on here. Add an example may be? (that it returns ['-f'] and ['--foo']... http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@277 PS2, Line 277: if short_opt in defaults: I think this part is subtle. It looks like we are updating defaults to also map from dest_name to default_val... May be add a line or two about it? -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 21 Mar 2019 22:06:43 +0000 Gerrit-HasComments: Yes
