You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Kim Jin Chul (Code Review)" <ge...@cloudera.org> on 2017/11/21 02:08:46 UTC

[Impala-ASF-CR] IMPALA-4506: Do not display "tip of the day" if --quiet is set

Kim Jin Chul has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8613


Change subject: IMPALA-4506: Do not display "tip of the day" if --quiet is set
......................................................................

IMPALA-4506: Do not display "tip of the day" if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 10 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/8613/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.

Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 4:

(1 comment)

Thanks, almost there!

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py@1356
PS4, Line 1356:   The last character of the message should not be a return.
nit: remove this sentence and condense the docstring to one line, as in the function just above this one.



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 16:13:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py@1440
PS4, Line 1440:         if call(['klist', '-s']) != 0:
> This looks wrong to me.
You're right. Thanks for finding the potential issue.



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2017 01:54:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   intro = get_welcome_string(options.verbose)
> Others may have a different opinion, but I don't think we should show the '
Your idea has been introduced by PS#2. Let's wait for other's opinions.



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2017 05:52:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Hello Philip Zeyliger, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8613

to look at the new patch set (#2).

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................

IMPALA-4506: Do not display some intro message if --quiet is set

PS#1: Do not display "tip of day" if --quiet is set
PS#2: Do not display some intro message if -- quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 20 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/8613/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8613/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8613/2//COMMIT_MSG@9
PS2, Line 9: Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
> Please elide these messages about specific patch sets by folding the most r
Done


http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   # Override query_options from config file with those specified on the command line.
> It seems like intro can still be no-empty at lines 1484 and 1488 in PS2.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 08:39:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 7:

@Jim and Philip, I appreciate your review.


-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Dec 2017 00:28:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.

Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 2:

(2 comments)

> (1 comment)
 > 
 > Thanks for the contribution!
 > 
 > I think the code looks fine, but we can go even farther and remove
 > the 'intro'/'welcome' entirely if --quiet is set, or at least that
 > would be my preference.

This fits my intuition, too.

http://gerrit.cloudera.org:8080/#/c/8613/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8613/2//COMMIT_MSG@9
PS2, Line 9: PS#1: Do not display "tip of day" if --quiet is set
Please elide these messages about specific patch sets by folding the most recent (and most accurate) one into the commit message itself, with no reference to which PS it came in.


http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:     intro = WELCOME_STRING
> Your idea has been introduced by PS#2. Let's wait for other's opinions.
It seems like intro can still be no-empty at lines 1484 and 1488 in PS2.



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Sun, 26 Nov 2017 22:01:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.

Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Nov 2017 14:57:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

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/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................

IMPALA-4506: Do not display some intro message if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Reviewed-on: http://gerrit.cloudera.org:8080/8613
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
---
M shell/impala_shell.py
1 file changed, 28 insertions(+), 14 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1547/


-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Nov 2017 14:57:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.

Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1353
PS3, Line 1353: def get_intro(options):
Please add a docstring to new functions.


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1359
PS3, Line 1359:                 "not secured by TLS.\nALL PASSWORDS WILL BE SENT IN THE CLEAR TO IMPALA.\n")
nit: long line


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1362
PS3, Line 1362:       intro += REFRESH_AFTER_CONNECT_DEPRECATION_WARNING
nit, not your bug (already present in code): Can you make the last character of the return value (if there is one) always '\n' or never '\n'?


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1364
PS3, Line 1364: else:
              :     return ""
You could remove a level of indentation for the bulk of the function by making the first two non-docstring lines be

    if not options.verbose:
      return ""



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 14:56:14 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Hello Jim Apple, Philip Zeyliger, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8613

to look at the new patch set (#4).

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................

IMPALA-4506: Do not display some intro message if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 35 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/8613/4
-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 6: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Nov 2017 18:30:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Hello Jim Apple, Philip Zeyliger, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8613

to look at the new patch set (#3).

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................

IMPALA-4506: Do not display some intro message if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 31 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/8613/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.

Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1353
PS3, Line 1353: def get_intro(options):
> Please add a docstring to new functions.
Done


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1359
PS3, Line 1359:                 "not secured by TLS.\nALL PASSWORDS WILL BE SENT IN THE CLEAR TO IMPALA.\n")
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1362
PS3, Line 1362:       intro += REFRESH_AFTER_CONNECT_DEPRECATION_WARNING
> nit, not your bug (already present in code): Can you make the last characte
Done, last character should not be a return.


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1364
PS3, Line 1364: else:
              :     return ""
> You could remove a level of indentation for the bulk of the function by mak
Done. It's better readability.



-- 
To view, visit http://gerrit.cloudera.org:8080/8613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 15:15:10 +0000
Gerrit-HasComments: Yes