Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9291 )

Change subject: IMPALA-6508: add KRPC test flag
......................................................................


Patch Set 4:

(4 comments)

Thanks for the review, please see my inline comments and PS5.

http://gerrit.cloudera.org:8080/#/c/9291/4/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/9291/4/bin/start-impala-cluster.py@57
PS4, Line 57: krpc
> KRPC
Done


http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/custom_cluster_test_suite.py@138
PS4, Line 138: "--impalad_args=-use_krpc"
> Not quite following what this translates to ? For now, I have been using --
Yes, -use_krpc is a boolean and as such its presence enables it. This page has 
all four variations that are supported for a boolean flag: 
http://gflags.github.io/gflags/

 app_containing_foo --big_menu
 app_containing_foo --nobig_menu
 app_containing_foo --big_menu=true
 app_containing_foo --big_menu=false

I picked the single-dash variant because I've seen this convention elsewhere I 
think. Let me know if you prefer two dashes.


http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/skip.py
File tests/common/skip.py:

http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/skip.py@89
PS4, Line 89: krpc
> nit: KRPC
Done


http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/test_skip.py
File tests/common/test_skip.py:

http://gerrit.cloudera.org:8080/#/c/9291/4/tests/common/test_skip.py@24
PS4, Line 24: class TestSkipIf(ImpalaTestSuite):
> Is this test gonna be part of the patch or is it for testing only ?
My idea was to keep it in this patch so that we actually have a test for the 
things I added. Once we add a proper (non-custom-cluster) test using the 
SkipIfs I plan to drop this test again.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie01a5de2afac4a0f43d5fceff283f6108ad6a3ab
Gerrit-Change-Number: 9291
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 04:07:55 +0000
Gerrit-HasComments: Yes

Reply via email to