You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/06/03 18:00:37 UTC

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled 

Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................

IMPALA-3628: Fix cancellation from shell when security is enabled

To cancel a query, the shell will create a separate connection inside
it's SIGINT handler, and send the cancellation RPC. However this
connection did not start a secure connection if it needed to, meaning
that the cancellation attempt would just hang.

A workaround is to kill the shell process, which I expect is what users
have been doing with this bug which has been around since 2014.

Testing: I added a custom cluster test that starts Impala with SSL
enabled, and wrote two tests - one just to check SSL connectivity, and
the other to mimic the existing test_cancellation which sends SIGINT to
the shell process. In doing so I refactored the shell testing code a bit
so that all tests use a single ImpalaShell object, rather than rolling
their own Popen() based approaches when they needed to do something
unusual, like cancel a query.

In the cancellation test on my machine, SIGINT can take a few tries to
be effective. I'm not sure if this is a timing thing - perhaps the
Python interpreter doesn't correctly pass signals through to a handler
if it's in a blocking call, for example. The test reliably passes within
~5 tries on my machine, so the test tries 30 times, once per second.

Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
---
M shell/impala_shell.py
A tests/custom_cluster/test_client_ssl.py
A tests/shell/__init__.py
D tests/shell/impala_shell_results.py
M tests/shell/test_shell_commandline.py
D tests/shell/test_shell_common.py
M tests/shell/test_shell_interactive.py
A tests/shell/util.py
8 files changed, 263 insertions(+), 186 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#4).

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................

IMPALA-3628: Fix cancellation from shell when security is enabled

To cancel a query, the shell will create a separate connection inside
it's SIGINT handler, and send the cancellation RPC. However this
connection did not start a secure connection if it needed to, meaning
that the cancellation attempt would just hang.

A workaround is to kill the shell process, which I expect is what users
have been doing with this bug which has been around since 2014.

Testing:

I added a custom cluster test that starts Impala with SSL
enabled, and wrote two tests - one just to check SSL connectivity, and
the other to mimic the existing test_cancellation which sends SIGINT to
the shell process. In doing so I refactored the shell testing code a bit
so that all tests use a single ImpalaShell object, rather than rolling
their own Popen() based approaches when they needed to do something
unusual, like cancel a query.

In the cancellation test on my machine, SIGINT can take a few tries to
be effective. I'm not sure if this is a timing thing - perhaps the
Python interpreter doesn't correctly pass signals through to a handler
if it's in a blocking call, for example. The test reliably passes within
~5 tries on my machine, so the test tries 30 times, once per second.

Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
---
M shell/impala_shell.py
A tests/custom_cluster/test_client_ssl.py
A tests/shell/__init__.py
D tests/shell/impala_shell_results.py
M tests/shell/test_shell_commandline.py
D tests/shell/test_shell_common.py
M tests/shell/test_shell_interactive.py
A tests/shell/util.py
8 files changed, 274 insertions(+), 191 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


Patch Set 1:

(3 comments)

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

PS1, Line 638:     # If the connection fails and the Kerberos has not been enabled,
             :     # check for a valid kerberos ticket and retry the connection
             :     # with kerberos enabled.
             :     if not self.imp_client.connected and not self.use_kerberos:
             :       try:
             :         if call(["klist", "-s"]) == 0:
             :           print_to_stderr(("Kerberos ticket found in the credentials cache, retrying "
             :                            "the connection with a secure transport."))
             :           self.imp_client.use_kerberos = True
             :           self.imp_client.use_ldap = False
             :           self.imp_client.ldap_password = None
             :           self._connect()
             :       except OSError, e:
             :         pass
If this code path is being hit then we might want to try this as well on the cancel path if we fail the first time.


http://gerrit.cloudera.org:8080/#/c/3302/1/tests/custom_cluster/test_client_ssl.py
File tests/custom_cluster/test_client_ssl.py:

PS1, Line 57: select count(*) from tpch.lineitem a join tpch.lineitem b "
            :                "join tpch.lineitem c
Any reason not to use a WAIT debug action?


PS1, Line 61: print
Should go through a logger (here and below), e.g.

 
LOG = logging.getLogger('name')
LOG.debug()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


Patch Set 1:

(3 comments)

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

PS1, Line 638:     # If the connection fails and the Kerberos has not been enabled,
             :     # check for a valid kerberos ticket and retry the connection
             :     # with kerberos enabled.
             :     if not self.imp_client.connected and not self.use_kerberos:
             :       try:
             :         if call(["klist", "-s"]) == 0:
             :           print_to_stderr(("Kerberos ticket found in the credentials cache, retrying "
             :                            "the connection with a secure transport."))
             :           self.imp_client.use_kerberos = True
             :           self.imp_client.use_ldap = False
             :           self.imp_client.ldap_password = None
             :           self._connect()
             :       except OSError, e:
             :         pass
> If this code path is being hit then we might want to try this as well on th
Done - by setting the class-wide variables so that next time _new_impala_client() is called, it will try to use Kerberos instead.


http://gerrit.cloudera.org:8080/#/c/3302/1/tests/custom_cluster/test_client_ssl.py
File tests/custom_cluster/test_client_ssl.py:

PS1, Line 57: select count(*) from tpch.lineitem a join tpch.lineitem b "
            :                "join tpch.lineitem c
> Any reason not to use a WAIT debug action?
Done


PS1, Line 61: print
> Should go through a logger (here and below), e.g.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


Patch Set 5: Code-Review+2 Verified+1

Private build passed: http://sandbox.jenkins.cloudera.com/job/impala-umbrella-build-and-test/2331/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


Patch Set 1:

Ping. This is an annoying shell bug.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#3).

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................

IMPALA-3628: Fix cancellation from shell when security is enabled

To cancel a query, the shell will create a separate connection inside
it's SIGINT handler, and send the cancellation RPC. However this
connection did not start a secure connection if it needed to, meaning
that the cancellation attempt would just hang.

A workaround is to kill the shell process, which I expect is what users
have been doing with this bug which has been around since 2014.

Testing:

I added a custom cluster test that starts Impala with SSL
enabled, and wrote two tests - one just to check SSL connectivity, and
the other to mimic the existing test_cancellation which sends SIGINT to
the shell process. In doing so I refactored the shell testing code a bit
so that all tests use a single ImpalaShell object, rather than rolling
their own Popen() based approaches when they needed to do something
unusual, like cancel a query.

In the cancellation test on my machine, SIGINT can take a few tries to
be effective. I'm not sure if this is a timing thing - perhaps the
Python interpreter doesn't correctly pass signals through to a handler
if it's in a blocking call, for example. The test reliably passes within
~5 tries on my machine, so the test tries 30 times, once per second.

Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
---
M shell/impala_shell.py
A tests/custom_cluster/test_client_ssl.py
A tests/shell/__init__.py
D tests/shell/impala_shell_results.py
M tests/shell/test_shell_commandline.py
D tests/shell/test_shell_common.py
M tests/shell/test_shell_interactive.py
A tests/shell/util.py
8 files changed, 271 insertions(+), 191 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3628: Fix cancellation from shell when security is enabled

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-3628: Fix cancellation from shell when security is enabled
......................................................................


IMPALA-3628: Fix cancellation from shell when security is enabled

To cancel a query, the shell will create a separate connection inside
it's SIGINT handler, and send the cancellation RPC. However this
connection did not start a secure connection if it needed to, meaning
that the cancellation attempt would just hang.

A workaround is to kill the shell process, which I expect is what users
have been doing with this bug which has been around since 2014.

Testing:

I added a custom cluster test that starts Impala with SSL
enabled, and wrote two tests - one just to check SSL connectivity, and
the other to mimic the existing test_cancellation which sends SIGINT to
the shell process. In doing so I refactored the shell testing code a bit
so that all tests use a single ImpalaShell object, rather than rolling
their own Popen() based approaches when they needed to do something
unusual, like cancel a query.

In the cancellation test on my machine, SIGINT can take a few tries to
be effective. I'm not sure if this is a timing thing - perhaps the
Python interpreter doesn't correctly pass signals through to a handler
if it's in a blocking call, for example. The test reliably passes within
~5 tries on my machine, so the test tries 30 times, once per second.

Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Reviewed-on: http://gerrit.cloudera.org:8080/3302
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Henry Robinson <he...@cloudera.com>
---
M shell/impala_shell.py
A tests/custom_cluster/test_client_ssl.py
A tests/shell/__init__.py
D tests/shell/impala_shell_results.py
M tests/shell/test_shell_commandline.py
D tests/shell/test_shell_common.py
M tests/shell/test_shell_interactive.py
A tests/shell/util.py
8 files changed, 274 insertions(+), 191 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If99085e75708d92a08dbecf0131a2234fedad33a
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>