Vincent Tran has posted comments on this change. ( http://gerrit.cloudera.org:8080/10747 )
Change subject: IMPALA-7181: Fix flaky test shell/test_shell_commandline.py::TestImpalaShell::test_socket_opening ...................................................................... Patch Set 1: (3 comments) Thanks for reviewing! http://gerrit.cloudera.org:8080/#/c/10747/1/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/10747/1/tests/shell/test_shell_commandline.py@629 PS1, Line 629: # Verify that impala-shell tries to create a socket against the host:port : # combination specified by -i when -b is not used : impala_shell = Popen(shlex.split("%s %s" % (shell_cmd, args1, )), stdout=devnull, : stderr=devnull) : connection, client_address = s.accept() : data = connection.recv(1024) : assert expected_output in data : impala_shell.kill() : connection.close() : : # Verify that impala-shell tries to create a socket against the host:port : # combination specified by -i when -b is used > I would prefer if the duplicated logic in the two cases would be merged to Good idea. Done. http://gerrit.cloudera.org:8080/#/c/10747/1/tests/shell/test_shell_commandline.py@645 PS1, Line 645: expected_output = "PingImpalaService" > Already set in line 627, no need to set here again. Yep. Thought I cleaned this up but it snuck back in after a few edits. http://gerrit.cloudera.org:8080/#/c/10747/1/tests/shell/test_shell_commandline.py@649 PS1, Line 649: s.close() > I am a bit worried about exceptions - this could be moved to a finally bloc Done -- To view, visit http://gerrit.cloudera.org:8080/10747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd64632ded936d49fc404bcac75588dd7886be44 Gerrit-Change-Number: 10747 Gerrit-PatchSet: 1 Gerrit-Owner: Vincent Tran <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vincent Tran <[email protected]> Gerrit-Comment-Date: Mon, 18 Jun 2018 19:05:59 +0000 Gerrit-HasComments: Yes
