Csaba Ringhofer 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) 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 a function somehow, but this is optional. 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. 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 block or the socket could be added to a with statement (see the python 2 version in https://stackoverflow.com/a/16772515 ) -- 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-Comment-Date: Mon, 18 Jun 2018 18:18:18 +0000 Gerrit-HasComments: Yes
